Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Setup release process and other improvements #4
Setup release process and other improvements #4
Changes from 7 commits
3adb58e
46ff8ce
4045e90
6c59493
1366f4e
cb1b930
1baa755
6b774ac
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Again, I would factor out the common condition:
although I doubt that you need the
github.event.action
check, because I don't think this will run if neither of those is true, per the check at line 21.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.
I need the
github.event.action
check as I am performing two different checks based on that result:github.event.action == 'closed'
--> checkgithub.event.pull_request.labels.*.name
github.event.action == 'labeled'
--> checkgithub.event.label.name
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.
Ah, I see.
But, if
github.event.action == 'labeled'
is true, thengithub.event.pull_request.labels.*.name
should contain the updated value (right?), so you can check that instead ofgithub.event.label.name
, which should let you simplify this to a single condition.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.
That's true, but if I check
github.event.pull_request.labels.*.name
when adding a label I am actually checking if at least one label with that name exists, which is true even if I am adding a new label andbackport
already exists on that pul request. This is not what we want as it would trigger the workflow multiple times.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.
Good point...but, I think that applies to my previous suggestion (which I shall withdraw 🙂).
In this spot, we've already decided to run the workflow, so the only question is whether to squash or not, and for that I think you can check
github.event.pull_request.labels.*.name
in either case.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.
See other comment #4 (comment) I don't think that is enough to check
backport
word. Anyway we are talking about minors so not a big dealThere 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.
It's...suboptimal...to have this duplicated here and in
.github/workflows/check-openapi-change.yaml
. You should consider refactoring this into a common "reusable workflow".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.
yeah I agree, but since we are still setting up most of the things I would leave this optimization in a followup PR (I will open an issue to keep track)
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.
I concur with trying to avoid "extra work", but I wouldn't regard using common workflows as an optimization -- rather, it's a savings, because you don't have to "reinvent the wheel", and if something needs updating then you can do it in one place, rather than having to track down all the copies of it.
This file was deleted.
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.
Here's that duplicated workflow again.
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.
As commented here #4 (comment)
This file was deleted.