-
Notifications
You must be signed in to change notification settings - Fork 54
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
feat(release workflows): Improve Release workflows to enable bugfix versions #1543
feat(release workflows): Improve Release workflows to enable bugfix versions #1543
Conversation
@paullatzelsperger |
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.
minor remarks, looks good to me otherwise
.github/workflows/draft-release.yaml
Outdated
outputs: | ||
branch_name: ${{ steps.release.outputs.branch_name || steps.bugfix.outputs.branch_name }} | ||
steps: | ||
- id: release |
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.
Personally, I prefer a simple shell script over conditional steps or dedicated actions, because shell scripts can be easily replicated in a local environment and are generally more easily understandable.
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 refactored the validation jobs in 788baa1 since I also agree those could be simpler.
On a personal level, I prefer the conditionals on the jobs or actions. The workflow execution graph is calculated before it is sent to the runner and conditionals that evaluate to false are skipped. When everything is in a shell script you have to be more careful what behaviors should lead to job failure and explicitly have it fail.
I'm fine either way. Let me know if the refactor is ok.
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.
We've seen this numerous times, where we want to replicate the runner's behaviour and debug or test a workflow locally, e.g. after a refactoring or a maintenance change. If everything is a job with conditional execution, as opposed to a simple script in a single job, there is zero chance of doing that, and ACT only gets you so far. Maybe I'm just old and like bash scripting.
Let's get this merged as-is so as not to block the PR.
Generally speaking, I would be very cautious when adding GH actions, because we are effectively introducing undebuggable foreign code, that you have no control over.
check-head: | ||
name: "Check if repository is not fork AND head is release OR base is bugfix" | ||
runs-on: ubuntu-latest | ||
if: ${{ github.repository == 'eclipse-tractusx/tractusx-edc' && (startsWith(github.ref_name, 'bugfix/') || startsWith(github.event.pull_request.head.ref, 'release/')) }} |
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.
this looks complicated plus it will be evaluated for every PR closed. given that we're following a sort-of gitflow approach, why don't we release on the release
branch and at the end we merge it back to main
later? this will simplify things a little.
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.
Yes, it will be evaluated for every merged PR on main
but it won't run because of the condition. And since this is a job-level conditional, github will just skip its execution and not throw any error (btw, that is the reason why I prefer job/action level conditionals vs everything script).
Like such.
If we release from the release
branch we can't create the tag from it, otherwise the release branch cannot be deleted.
I wouldn't say it looks complicated. Its actually very readable. It's just a veeeery long line 😅
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.
well, the tag can be created after the release branch has been merged into main. I think that's the typical gitflow approach
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 don't think I got your point then, sorry.
Your suggestion is to change the trigger of the release workflow to be the push on the release
branch?
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.
Either having this workflow to run when a PR is closed on a release
branch or making it manual as for the bugfix version.
At the end of the release we could merge back to main if ref_name starts with release
and snapshot version can be bumped.
The more the release
and bugfix
processes are similar, the easier will be to maintain them
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.
The workflow is being triggered when a PR is closed on a release
branch. If we really want to avoid the workflow to be evalutated on every closed PR to main, we can make it manual only.
1574727
to
68aa2df
Compare
68aa2df
to
129b34e
Compare
Quality Gate passedIssues Measures |
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.
let's try it out like this, still not really convinced about having the release workflow created for every pr merged but let's eventually change that later on
WHAT
Implements the decisions made in the following DR: https://github.com/eclipse-tractusx/tractusx-edc/tree/main/docs/development/decision-records/2024-08-28-Release-Workflow-Refactor
main
branch as base instead ofreleases
. No need to be moving commits around from releases and then to main.publish-tag-and-release
was created to enable a parametrized execution of the previous GH tag and release set of steps, depending if its a latest or bugfix release.gradle.properties
and not from branch names.WHY
releases/
branch and evenrelease/*
branches.Closes #1448