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

ci: remove "Log PR Number" step #172

Open
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

v-juliana
Copy link

Summary

It just removes a dangerous string interpolation from the Workflow.

@v-juliana v-juliana changed the base branch from main to development March 8, 2023 21:43
Copy link

@einsteinx2 einsteinx2 left a comment

Choose a reason for hiding this comment

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

It looks like you also adjusted the spacing in the same PR as the other changes you made which makes it really hard to compare the actual changes. I don't mind the spacing change, but if you're going to do that, please do it in a separate commit so that I can review the commit with the logical changes first. Same goes for the changes from single to double quotes as that also adds noise.

Please split the logic and formatting changes into two commits and force push to this branch.

@v-juliana
Copy link
Author

It looks like you also adjusted the spacing in the same PR as the other changes you made which makes it really hard to compare the actual changes. I don't mind the spacing change, but if you're going to do that, please do it in a separate commit so that I can review the commit with the logical changes first. Same goes for the changes from single to double quotes as that also adds noise.

Please split the logic and formatting changes into two commits and force push to this branch.

Sorry for that, it was code applying the autoformatting.

@jasonmgeorge
Copy link

Would it be possible to address by assigning these function calls to an intermediate variable per Using an intermediate environment variable?

Also, I'm not clear on why the script was set up to use find-pr-title and find-pr-number to parse data as opposed to using ${{ github.event.pull_request.title }} and ${{ github.event.pull_request.number }}. Seems like the complexity could be reduced if event is available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants