fix(INFRA-3037): refactor, remove release scripts dups #195
Conversation
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
bugbot run |
|
bugbot run |
|
bugbot run |
There was a problem hiding this comment.
Pull request overview
This PR refactors the release automation scripts by extracting shared utility functions into a centralized location, improving code maintainability and reducing duplication across release workflows.
Key Changes:
- Extract shared git/branch/PR helper functions into a reusable utilities module
- Create standalone changelog updater script with enhanced parameterization
- Improve branch and version handling with better defaults and validation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
.github/scripts/utils.sh |
New shared utilities file containing reusable functions (configure_git, checkout_or_create_branch, push_branch_with_handling, create_pr_if_not_exists) for release scripts |
.github/scripts/update-release-changelog.sh |
Updated to source shared utilities, add support for custom changelog branch and explicit version parameters, and improve documentation |
.github/scripts/create-platform-release-pr.sh |
Refactored to source and use shared utilities, delegate changelog operations to standalone script, export git identity via environment variables, and add BASE_BRANCH parameterization |
Comments suppressed due to low confidence (1)
.github/scripts/create-platform-release-pr.sh:49
- The
BASE_BRANCHvariable is assigned on line 40 but is not included in the debug logging section (lines 42-49). For consistency with other variables and to aid in debugging, consider adding it to the log output:
echo "BASE_BRANCH: $BASE_BRANCH"BASE_BRANCH="${BASE_BRANCH:-main}"
# Log assigned variables for debugging (after defaults and trimming)
echo "Assigned variables:"
echo "PLATFORM: $PLATFORM"
echo "PREVIOUS_VERSION_REF: $PREVIOUS_VERSION_REF"
echo "NEW_VERSION: $NEW_VERSION"
echo "NEW_VERSION_NUMBER: $NEW_VERSION_NUMBER"
echo "GIT_USER_NAME: $GIT_USER_NAME"
echo "GIT_USER_EMAIL: $GIT_USER_EMAIL"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
bugbot run |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jluque0101
force-pushed
the
chore/INFRA-3067-ref-duplication
branch
from
66ba953 to
e5a2948
Compare
jluque0101
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if git show-ref --verify --quiet "refs/heads/${branch_name}" || git ls-remote --heads origin "${branch_name}" | grep -q "${branch_name}"; then | ||
| echo "Branch ${branch_name} already exists, checking it out" | ||
| if git ls-remote --heads origin "${branch_name}" | grep -q "${branch_name}"; then | ||
| git fetch origin "${branch_name}" | ||
| git checkout "${branch_name}" | ||
| else | ||
| git checkout "${branch_name}" | ||
| fi |
There was a problem hiding this comment.
The branch existence check is redundant and inefficient. The condition checks both local (git show-ref) and remote (git ls-remote) existence, then the inner if-else checks remote existence again. Consider simplifying to check remote first (which will cover both cases in a distributed workflow), or restructure the logic to avoid the duplicate remote check on line 25.
| if git show-ref --verify --quiet "refs/heads/${branch_name}" || git ls-remote --heads origin "${branch_name}" | grep -q "${branch_name}"; then | |
| echo "Branch ${branch_name} already exists, checking it out" | |
| if git ls-remote --heads origin "${branch_name}" | grep -q "${branch_name}"; then | |
| git fetch origin "${branch_name}" | |
| git checkout "${branch_name}" | |
| else | |
| git checkout "${branch_name}" | |
| fi | |
| if git ls-remote --heads origin "${branch_name}" | grep -q "${branch_name}"; then | |
| echo "Branch ${branch_name} already exists on remote, checking it out" | |
| git fetch origin "${branch_name}" | |
| git checkout "${branch_name}" | |
| elif git show-ref --verify --quiet "refs/heads/${branch_name}"; then | |
| echo "Branch ${branch_name} already exists locally, checking it out" | |
| git checkout "${branch_name}" |
There was a problem hiding this comment.
This is not essential, I would prefer to keep this logic 'as it is' rather than adding more changes on top of the refactor, we could treat it in a separate ticket however
| "${SCRIPT_DIR}/update-release-changelog.sh" \ | ||
| "${release_branch_name}" \ | ||
| "${platform}" \ | ||
| "${GITHUB_REPOSITORY_URL}" \ | ||
| "${previous_version_ref}" \ | ||
| "${changelog_branch_name}" \ | ||
| "${new_version}" |
There was a problem hiding this comment.
Missing validation for GITHUB_REPOSITORY_URL environment variable. The script is called with GITHUB_REPOSITORY_URL on line 318, but there's no check to ensure this environment variable is set when TEST_ONLY is false. According to the documentation at line 25, this is required for changelog generation. Consider adding a validation check similar to other required parameters.
There was a problem hiding this comment.
I'd like to avoid scope creep
env:
GITHUB_REPOSITORY_URL: '${{ github.server_url }}/${{ github.repository }}'will be always set , it would be also a redundant protection as the child script checks for the REPOSITORY_URL too.
Still, technically correct because it would 'fail fast' (main vs child script)
What is the current state of things and why does it need to change?
What is the solution your changes offer and how does it work?
utils.shcreate-platform-release-pr.shtoupdate-release-changelog.shAre there any issues or other links reviewers should consult to understand this pull request better? For instance:
See:
https://github.com/consensys-test/metamask-extension-infra-3037
https://github.com/consensys-test/metamask-mobile-infra-3037
Note
Consolidates release automation by extracting shared helpers and moving changelog logic to a standalone script.
utils.shwith sharedconfigure_git,checkout_or_create_branch,push_branch_with_handling, andcreate_pr_if_not_existsused across scriptsupdate-release-changelog.shto generate/update changelog branches and draft PRs (syncs release branch, chooses existingrelease/<version>-Changelogorchore/<version>-Changelog, commits/pushes, creates PR); removescommits.csvgeneration from this flowcreate-platform-release-pr.shto sourceutils.sh, supportBASE_BRANCH, delegate changelog PR creation toupdate-release-changelog.sh, pass git identity via env, and improve argument/env handling (e.g.,previous_version_ref/hotfix and test mode)Written by Cursor Bugbot for commit e5a2948. This will update automatically on new commits. Configure here.