Uh oh!
There was an error while loading. Please reload this page.
INFRA-2911-Skip generating commits.csv for hotfixes - #118
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
gauthierpetetin
left a comment
There was a problem hiding this comment.
Thank you for this new PR @XxdpavelxX , I added a few comments
Uh oh!
There was an error while loading. Please reload this page.
| fi | ||
| # Note: Skip PREVIOUS_VERSION_REF validation to allow empty for hotfixes |
There was a problem hiding this comment.
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.
It was just a general comment to explain the behavior for hotfixes, but I'll remove it.
| 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.
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.
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.
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'.
| # 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.
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.
Good catch. A little confused though, should I hardcode it to "main" or "stable" here?
There was a problem hiding this comment.
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
bb2b66dComparegauthierpetetin
left a comment
There was a problem hiding this comment.
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.
a new major/minor releases --> release without an "s"
| 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.
a new hotfix releases --> release without an "s"
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