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.

Sign up for GitHub

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

Jump to bottom

ci: simplify if statements in workflows #1159

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

ci: simplify if statements in workflows #1159

wants to merge 1 commit into from

Conversation

katexochen
Copy link
Member

No description provided.

@katexochen katexochen added the no changelog PRs not listed in the release notes label Jan 16, 2025
@katexochen katexochen requested a review from msanft January 17, 2025 15:05
msanft
msanft approved these changes Jan 17, 2025
View reviewed changes
Copy link
Contributor

@msanft msanft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I roughly have in mind that bracing these like we did before was necessary for some reason. Do you still remember? Generally, if the CI continues to work as expected (which it seems to do based on the tests run on this PR), I'm fine with the change.

.github/workflows/e2e.yml
@@ -33,7 +33,7 @@ jobs:
packages: write
steps:
- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
- if: ${{ !inputs.self-hosted }}
- if: (!inputs.self-hosted)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the parentheses here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, otherwise it's interpreted as yaml foo

@katexochen
Copy link
Member Author

I roughly have in mind that bracing these like we did before was necessary for some reason. Do you still remember? Generally, if the CI continues to work as expected (which it seems to do based on the tests run on this PR), I'm fine with the change.

If is always interpreted, so braces aren't needed.

@msanft
Copy link
Contributor

msanft commented Jan 17, 2025

If is always interpreted, so braces aren't needed.

Thanks! Good to merge for me then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msanft msanft msanft approved these changes

Assignees
No one assigned
Labels
no changelog PRs not listed in the release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@katexochen @msanft