-
-
Notifications
You must be signed in to change notification settings - Fork 4
INFRA-2911-Skip generating commits.csv for hotfixes #118
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
Conversation
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.
Thank you for this new PR @XxdpavelxX , I added a few comments
| fi | ||
|
|
||
|
|
||
| # Note: Skip PREVIOUS_VERSION_REF validation to allow empty for hotfixes |
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.
Why do we place this comment here? It seems we don't have related logic close to it.
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.
It was just a general comment to explain the behavior for hotfixes, but I'll remove it.
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.
ok!
| git fetch origin "${previous_version_ref}" | ||
| DIFF_BASE="origin/${previous_version_ref}" | ||
| # Skip commits.csv for hotfix releases (previous_version_ref empty) | ||
| local skip_csv=false |
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'm not sure we need skip_csv variable for our 'if' conditions below, since we can also base our 'if' conditions on the state of previous_version_ref variable.
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.
Sure, I can replace conditionals using skip_csv=true/false with previous_version_ref='null' if that works?
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, that works for me, and we could add a detailed comment to explain why, such as:
- When we create a new major/minor releases, we fetch all commits included in the release, by fetching the diff between HEAD and previous version reference.
- When we create a new hotfix releases, there are no commits included in the release by default (they will be cherry-picked one by one). So we don't have previous version reference, which is why the value is set to 'null'.
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.
Done
|
|
||
| # Step 3: Create version bump PR for main branch | ||
| create_version_bump_pr "$PLATFORM" "$NEW_VERSION" "$next_version" "$version_bump_branch_name" "$release_branch_name" "main" | ||
| create_version_bump_pr "$PLATFORM" "$NEW_VERSION" "$next_version" "$version_bump_branch_name" "$release_branch_name" "$BASE_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.
I don't think we can pass "BASE_BRANCH" instead of "main" here, as "BASE_BRANCH" is meant to be the base branch of the release PR, which is always set to stable.
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 catch. A little confused though, should I hardcode it to "main" or "stable" here?
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.
You should hardcode it to "main", as we'll bump the version on main.
Signed-off-by: Pavel Dvorkin <pavel.dvorkin@consensys.net>
c3a0059 to
bb2b66d
Compare
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 added two nits to fix. Other than that LGTM
| echo "Hotfix release detected (previous-version-ref empty); skipping commits.csv generation." | ||
| skip_csv=true | ||
| # Skip commits.csv for hotfix releases (previous_version_ref is literal "null") | ||
| # - When we create a new major/minor releases, we fetch all commits included in the release, by fetching the diff between HEAD and previous version reference. |
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.
a new major/minor releases --> release without an "s"
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.
Fixed
| skip_csv=true | ||
| # Skip commits.csv for hotfix releases (previous_version_ref is literal "null") | ||
| # - When we create a new major/minor releases, we fetch all commits included in the release, by fetching the diff between HEAD and previous version reference. | ||
| # - When we create a new hotfix releases, there are no commits included in the release by default (they will be cherry-picked one by one). So we don't have previous version reference, which is why the value is set to 'null'. |
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.
a new hotfix releases --> release without an "s"
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.
Fixed
Ticket: https://consensyssoftware.atlassian.net/browse/INFRA-2911
Skip generating commits.csv for hotfixes on create-release-pr workflow
Ticket: https://consensyssoftware.atlassian.net/browse/INFRA-2911
Testing:
Manually triggered major release: https://github.com/consensys-test/metamask-extension-test-workflow2/actions/runs/17496896766/job/49700017533
Manually triggered hotfix: https://github.com/consensys-test/metamask-extension-test-workflow2/actions/runs/17504431701/job/49724856275
Automated triggered major release: https://github.com/consensys-test/metamask-extension-test-workflow2/actions/runs/17502784236
Automated triggered hotfix: (skipped): https://github.com/consensys-test/metamask-extension-test-workflow2/actions/runs/17497053913
Automated major release after hotfix: https://github.com/consensys-test/metamask-extension-test-workflow2/actions/runs/17504530690