Skip to content
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

Merged
merged 2 commits into from
Oct 19, 2022
Merged

Fix ci outputs #14662

merged 2 commits into from
Oct 19, 2022

Conversation

nineinchnick
Copy link
Member

@nineinchnick nineinchnick commented Oct 17, 2022

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:

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
Copy link
Contributor

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?

Copy link
Member Author

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.


- 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' }}
Copy link
Contributor

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")

Copy link
Member Author

@nineinchnick nineinchnick Oct 17, 2022

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:
image

and then you can see how it gets read into Python in the next step:
image

@hashhar
Copy link
Member

hashhar commented Oct 17, 2022

I've run CI with secrets here - #14664

Copy link
Member

@hashhar hashhar left a 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)

@nineinchnick
Copy link
Member Author

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)

All done!

@nineinchnick
Copy link
Member Author

@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?

@nineinchnick
Copy link
Member Author

@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.
@hashhar
Copy link
Member

hashhar commented Oct 18, 2022

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).

@nineinchnick
Copy link
Member Author

Thanks, looks like it's finally working: https://github.com/trinodb/trino/actions/runs/3271424004/jobs/5381195377

will try to send a follow-up to clean this complexity up if possible.

Do you mean doing something with these ignore exclusion if? That'd be nice, they're hard to read.

@hashhar
Copy link
Member

hashhar commented Oct 18, 2022

Do you mean doing something with these ignore exclusion if? That'd be nice, they're hard to read.

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).

@ebyhr
Copy link
Member

ebyhr commented Oct 19, 2022

@hashhar Can we merge this PR now?

@hashhar
Copy link
Member

hashhar commented Oct 19, 2022

@ebyhr Yes, I think so - I don't know if failures are related. Sorry I did not take a look at this recently.

@ebyhr
Copy link
Member

ebyhr commented Oct 19, 2022

Thanks for your confirmation. Let me merge so that we can run #14687 in Databricks.

@ebyhr ebyhr merged commit ddcb07d into trinodb:master Oct 19, 2022
@nineinchnick nineinchnick deleted the fix-ci-outputs branch October 19, 2022 16:50
@github-actions github-actions bot added this to the 401 milestone Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants