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 equivalence test actions #36023

Merged
merged 3 commits into from
Nov 18, 2024
Merged

Fix equivalence test actions #36023

merged 3 commits into from
Nov 18, 2024

Conversation

liamcervante
Copy link
Member

This PR fixes all the syntax errors in my first attempt at this.

@liamcervante liamcervante force-pushed the liamcervante/eq-test-fixed branch from 4c2f0cc to 670ae80 Compare November 18, 2024 13:19
@liamcervante liamcervante marked this pull request as ready for review November 18, 2024 13:19
@liamcervante liamcervante requested a review from a team as a code owner November 18, 2024 13:19
@liamcervante liamcervante force-pushed the liamcervante/eq-test-fixed branch from 670ae80 to 5fbd174 Compare November 18, 2024 13:50
merged = ${{ github.event.pull_request.merged }}
target_branch = ${{ github.event.pull_request.base.ref }}
merged=${{ github.event.pull_request.merged }}
target_branch=${{ github.event.pull_request.base.ref }}
Copy link
Member

Choose a reason for hiding this comment

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

We might want to enclose this in (single) quotes in case the branch name has spaces or something that needs escaping.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@liamcervante liamcervante force-pushed the liamcervante/eq-test-fixed branch 2 times, most recently from a80f879 to b54847b Compare November 18, 2024 13:58
Copy link
Contributor

The equivalence tests will be updated. Please verify the changes here.

Copy link
Contributor

The equivalence tests will be updated. Please verify the changes here.

Copy link
Contributor

The equivalence tests will be updated. Please verify the changes here.

Copy link
Contributor

The equivalence tests will be updated. Please verify the changes here.

Copy link
Contributor

The equivalence tests will be updated. Please verify the changes here.

Copy link
Contributor

The equivalence tests will be updated. Please verify the changes here.

@liamcervante liamcervante merged commit 840bc0c into main Nov 18, 2024
6 of 8 checks passed
@liamcervante liamcervante deleted the liamcervante/eq-test-fixed branch November 18, 2024 15:09
Copy link
Contributor

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

merged = ${{ github.event.pull_request.merged }}
target_branch = ${{ github.event.pull_request.base.ref }}
merged="${{ github.event.pull_request.merged }}"
target_branch="${{ github.event.pull_request.base.ref }}"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
target_branch="${{ github.event.pull_request.base.ref }}"
target_branch='${{ github.event.pull_request.base.ref }}'

I think this is slightly safer as it disallows interpolation, so that in case the ref contained some special characters, like $() then it won't have any special meaning if it's enclosed in ' rather than ". The only special character then becomes just '.

Hopefully GitHub disallows branch names with special characters anyway but better safe than sorry.

Copy link
Contributor

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants