-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix ci outputs #14662
Fix ci outputs #14662
Conversation
.github/workflows/ci.yml
Outdated
fi | ||
- name: Check GCP secrets | ||
id: check-gcp-secrets | ||
run: | | ||
if [[ "${{ secrets.GCP_CREDENTIALS_KEY }}" != "" ]]; \ | ||
then | ||
echo "Secrets to run GCP product tests were configured in the repo" | ||
echo "::set-output name=have_gcp_secrets::true" | ||
echo "have_gcp_secrets=true" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have yes
/no
instead of true
/false
to avoid misunderstandings / future confusions with boolean
type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In YAML, that's not a good idea, because no
also gets typecasted into a boolean, whereas yes
is not. So it's actually worse than using true
and false
where both get cast into a boolean if not quoted.
IMO in YAML, explicit quoting is the right thing to do and should be enough.
But here we already only treat this as a string.
.github/workflows/ci.yml
Outdated
|
||
- suite: suite-delta-lake-databricks | ||
config: hdp3 | ||
- suite: suite-delta-lake-databricks | ||
ignore exclusion if: >- | ||
${{ needs.build-pt.outputs.have_databricks_secrets == 'true' }} | ||
${{ needs.check-databricks-secrets.outputs.have_databricks_secrets == 'true' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The outcome in this case a boolean
right?
(ignore exclusion if
is set to "false"
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's a string. I think it might be caused by using the multiline block (using >-
, where >
is replacing newlines with spaces and -
chops the trailing newline).
You can see the final yaml after evaluating the GHA expressions in the step that writes it into the file:
and then you can see how it gets read into Python in the next step:
I've run CI with secrets here - #14664 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify in 2nd commit message that "Using output of a step within the same job doesn't work as expected/is not supposed to work" (and probably split out removal of set-output
in separate commit otherwise it's unclear to anyone in future what the issue was and it was unrelated to set-output being deprecated)
3c7b799
to
88432f8
Compare
All done! |
88432f8
to
e140efa
Compare
@hashhar I saw in your test PR with secrets that it still didn't work. I restored the outputs definition in the job, maybe it's required. Can you run it again? |
e140efa
to
e5b0f19
Compare
@hashhar It didn't work again. I simplified it by removing all vars and just checking secrets directly when rendering the matrix yaml. Please update your PR. |
Using output of a step within the same job is not supposed to work.
e5b0f19
to
36eeb04
Compare
Was a small syntax error - pushed a fix. Will merge after verifying things work as expected (and will try to send a follow-up to clean this complexity up if possible). |
Thanks, looks like it's finally working: https://github.com/trinodb/trino/actions/runs/3271424004/jobs/5381195377
Do you mean doing something with these |
Indeed. First of all that name is very misleading + I don't like how we try to emulate some boolean logic instead of actually showing how GHA matrix exclusions work (which is simpler than this "fake" boolean stuff and can be explained in a code comment + link to docs). |
@hashhar Can we merge this PR now? |
@ebyhr Yes, I think so - I don't know if failures are related. Sorry I did not take a look at this recently. |
Thanks for your confirmation. Let me merge so that we can run #14687 in Databricks. |
Description
Fix referring to outputs from the same job.
Stop using the deprecated
set-output
command: https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/Broken since #10984. Supersedes #14661
Non-technical explanation
n/a
Release notes
(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: