Conversation
berfinyuksel
had a problem deploying
to
production
GitHub Actions
Error
There was a problem hiding this comment.
Pull request overview
This PR enhances the GitHub Actions release workflow by improving code quality, fixing typos, and adding milestone management capabilities.
Changes:
- Fixed indentation and spelling errors throughout the workflow ("prerelase" → "prerelease", "exitence" → "existence")
- Added milestone management features: ability to close milestones, create new milestones, and move issues between milestones
- Improved shell script robustness with better error handling, input validation, and variable quoting
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if [[ "${GEN_STATUS}" -eq 200 ]]; then | ||
| GEN_NOTES=$(echo "${GEN_RESPONSE}" | sed '\$d' | jq -r '.body // ""') | ||
| FINAL_BODY="${GEN_NOTES}\n\n---\n\n${CHANGELOG}" |
There was a problem hiding this comment.
The newline escape sequence '\n' in FINAL_BODY will be interpreted literally by the shell, not as actual newlines. Consider using $'\n' syntax or a different approach to insert newlines, such as using actual newlines in the string or printf formatting.
| FINAL_BODY="${GEN_NOTES}\n\n---\n\n${CHANGELOG}" | |
| FINAL_BODY="${GEN_NOTES}"$'\n\n---\n\n'"${CHANGELOG}" |
| TARGET_REPO: ${{ needs.display-parameters.outputs.target_repo }} | ||
| LATEST: ${{ needs.display-parameters.outputs.latest }} | ||
| PRERELEASE: ${{ needs.display-parameters.outputs.prerelease }} | ||
| RELEASE_TOKEN: ${{ secrets.RELEASE_TOKEN }} |
There was a problem hiding this comment.
The RELEASE_TOKEN secret is now only available in the create-release job environment context (line 135), but it was previously available globally (removed from line 57). The milestone-maintenance job steps that need RELEASE_TOKEN declare it in their step-level env (lines 291, 343, 411), which is correct. However, this could be confusing - consider documenting why RELEASE_TOKEN is scoped differently or making it consistent across jobs.
| needs: create-release | ||
|
|
||
| # Run only when at least one milestone-related input is provided | ||
| if: ${{ needs.create-release.result == 'success' && (inputs.close_milestone != '' || inputs.new_milestone != '' || inputs.move_issues != '') }} |
There was a problem hiding this comment.
The condition uses legacy != '' syntax for checking non-empty strings. While this works, it's inconsistent with the bash-style [[ -n ]] checks used throughout the shell scripts. Consider using GitHub Actions' native syntax for consistency, or document the intentional use of != '' for readability.
| type: string | ||
| description: 'The tag name of the release with leading v' | ||
| release_type: | ||
| description: 'Choose relase type' |
There was a problem hiding this comment.
Typo in description: "relase" should be "release".
| description: 'Choose relase type' | |
| description: 'Choose release type' |
| options: | ||
| options: | ||
| - latest | ||
| - prerelase |
There was a problem hiding this comment.
Typo in option value: "prerelase" should be "prerelease".
| - prerelase | |
| - prerelease |
| # Normalize release_type (keep accepting "prerelase" input option) | ||
| if [[ "${RELEASE_TYPE}" == "prerelase" ]]; then |
There was a problem hiding this comment.
The code normalizes "prerelase" input but the option value still contains the typo. Users selecting "prerelase" from the choice menu will have their selection properly handled by the normalization logic on line 79, but the typo should be fixed in the option list to prevent confusion. Consider whether backwards compatibility is needed for this workflow_dispatch input.
| # Normalize release_type (keep accepting "prerelase" input option) | |
| if [[ "${RELEASE_TYPE}" == "prerelase" ]]; then | |
| # Normalize release_type (accept both legacy "prerelase" and correct "prerelease") | |
| if [[ "${RELEASE_TYPE}" == "prerelase" || "${RELEASE_TYPE}" == "prerelease" ]]; then |
|
|
||
| echo "Closed milestone '${INPUT}' (number: ${MS_NUMBER})" >> "$GITHUB_STEP_SUMMARY" | ||
|
|
||
| - name: Create new milestone (by title or number) if provided (reuse if exists) |
There was a problem hiding this comment.
Inconsistent comment style: This comment uses lowercase "by title or number" while the similar comment on line 287 uses the same format. However, the comment on line 338 says "by title or number" when it should clarify that providing a number means "reuse existing milestone" rather than creating by number. The behavior differs from the close_milestone parameter where a number directly references a milestone to close.
| - name: Create new milestone (by title or number) if provided (reuse if exists) | |
| - name: Create or reuse milestone (create by title, reuse by number) if provided |
berfinyuksel
had a problem deploying
to
production
GitHub Actions
Failure
| if [[ "${BASE}" == *.x ]]; then | ||
| BASE_MAJOR_PART="${BASE%%.*}" | ||
| if ! [[ "${BASE_MAJOR_PART}" =~ ^[0-9]+$ ]]; then | ||
| echo "Invalid base_branch format. Expected numeric major version, got: ${BASE}" >> "$GITHUB_STEP_SUMMARY" | ||
| exit 1 | ||
| fi | ||
| fi | ||
|
|
||
| TAG_NAME="v${TAG}" | ||
| RELEASE_NAME="${TAG}" | ||
|
|
||
| BASE_IS_X=false | ||
| if [[ "${BASE}" == *.x ]]; then | ||
| BASE_IS_X=true | ||
| fi |
There was a problem hiding this comment.
| if [[ "${BASE}" == *.x ]]; then | |
| BASE_MAJOR_PART="${BASE%%.*}" | |
| if ! [[ "${BASE_MAJOR_PART}" =~ ^[0-9]+$ ]]; then | |
| echo "Invalid base_branch format. Expected numeric major version, got: ${BASE}" >> "$GITHUB_STEP_SUMMARY" | |
| exit 1 | |
| fi | |
| fi | |
| TAG_NAME="v${TAG}" | |
| RELEASE_NAME="${TAG}" | |
| BASE_IS_X=false | |
| if [[ "${BASE}" == *.x ]]; then | |
| BASE_IS_X=true | |
| fi | |
| BASE_IS_X=false | |
| if [[ "${BASE}" == *.x ]]; then | |
| BASE_IS_X=true | |
| fi | |
| if $[[ "${BASE_IS_X}" == "true"]]; then | |
| BASE_MAJOR_PART="${BASE%%.*}" | |
| if ! [[ "${BASE_MAJOR_PART}" =~ ^[0-9]+$ ]]; then | |
| echo "Invalid base_branch format. Expected numeric major version, got: ${BASE}" >> "$GITHUB_STEP_SUMMARY" | |
| exit 1 | |
| fi | |
| fi | |
| TAG_NAME="v${TAG}" | |
| RELEASE_NAME="${TAG}" | |
micro-optimization since if [[ "${BASE}" == *.x ]]; then appeared twice in this block
There was a problem hiding this comment.
probably won't need to be quoted comparison for every if $[[ "${BASE_IS_X}" == "true"]];, just IF $BASE_IS_X; would be enough
| MAKE_LATEST=false | ||
| PRERELEASE=false | ||
|
|
||
| if [[ "${DRAFT}" == "false" ]]; then | ||
| if [[ "${RELEASE_KIND}" == "major" || "${RELEASE_KIND}" == "minor" ]]; then | ||
| MAKE_LATEST=true | ||
| fi | ||
| fi |
There was a problem hiding this comment.
MAKE_LATEST need some refactoring in the logic here, it is actually possible to tag a bugfix only and it would be the latest (eg. in bugfix only release cycle)
There was a problem hiding this comment.
Theoretically , it should be always LATEST and when we do releases in order, we start from bugfix and then minor and then major, or at least publish the draft in this order and it would be properly one on top of each other
kingjia90
left a comment
kingjia90
left a comment
There was a problem hiding this comment.
some inputs to avoid duplicated code
| permissions: | ||
| contents: write | ||
| needs: prepare | ||
| if: ${{ inputs.publish_immediately == false }} |
There was a problem hiding this comment.
the steps for draft and instant publish are mostly duplicated, making it more hard to maintain in case of changes, this IF should be removed
| if: ${{ inputs.publish_immediately == false }} |
and name adapted accordingly and changed as below
| LATEST_SHA: ${{ needs.prepare.outputs.latest_sha }} | ||
| TAG_NAME: ${{ needs.prepare.outputs.tag_name }} | ||
| RELEASE_NAME: ${{ needs.prepare.outputs.release_name }} | ||
| DRAFT: ${{ needs.prepare.outputs.draft }} |
There was a problem hiding this comment.
| DRAFT: ${{ needs.prepare.outputs.draft }} | |
| DRAFT: ${{ inputs.publish_immediately == false }} |
| if [[ "${COUNT}" -lt 3 ]]; then | ||
| echo "Tag '${TAG}' must have at least 3 parts for major/minor/bugfix." >> "$GITHUB_STEP_SUMMARY" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
this check could/should be removed here and done before the draft even being created
https://github.com/pimcore/workflows-collection-public/pull/60/files#diff-a318819fb0ec8889018cbef8e6dd222e317757d95ddd93b62b714ac7c49f04f6R117
in other words, this IF should be unreachable
by looking at the exit 0 on if [[ "${KIND}" == "hotfix" ]]; then, the code below this should in a else block
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| resolve_ms () { | ||
| local TITLE="$1" | ||
| local PAGE=1 | ||
| while true; do | ||
| local RESP | ||
| RESP=$(curl -s --location \ | ||
| "https://api.github.com/repos/${OWNER}/${REPO}/milestones?state=all&per_page=100&page=${PAGE}" \ | ||
| -H "X-GitHub-Api-Version: 2022-11-28" \ | ||
| -H "Accept: application/vnd.github+json" \ | ||
| -H "Authorization: Bearer ${RELEASE_TOKEN}") | ||
| local LEN | ||
| LEN=$(echo "${RESP}" | jq '. | length') | ||
| if [[ "${LEN}" -eq 0 ]]; then | ||
| echo "" | ||
| return 0 | ||
| fi | ||
| local NUM | ||
| NUM=$(echo "${RESP}" | jq -r --arg t "${TITLE}" '.[] | select(.title == $t) | .number' | head -n 1) | ||
| if [[ -n "${NUM}" && "${NUM}" != "null" ]]; then | ||
| echo "${NUM}" | ||
| return 0 | ||
| fi | ||
| PAGE=$((PAGE+1)) | ||
| if [[ "${PAGE}" -gt 20 ]]; then | ||
| echo "" | ||
| return 0 | ||
| fi | ||
| done | ||
| } |
There was a problem hiding this comment.
The milestone resolution logic at lines 664-692 is defined as a function within a single step. This pattern is repeated in multiple places with variations (lines 753-772, 603-626, 555-576). Consider consolidating this into a reusable utility function to reduce code duplication and improve maintainability.
| run: | | ||
| set -euo pipefail | ||
|
|
||
| if ! [[ "${REPOSITORY}" =~ ^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ ]]; then |
There was a problem hiding this comment.
The validation regex for repository format allows dots in repository names, which is non-standard for GitHub repositories. GitHub repository names can contain alphanumeric characters, hyphens, and underscores, but typically not dots. Consider if this is intentional or if the regex should be more restrictive.
| if ! [[ "${REPOSITORY}" =~ ^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ ]]; then | |
| if ! [[ "${REPOSITORY}" =~ ^[A-Za-z0-9_.-]+/[A-Za-z0-9_-]+$ ]]; then |
| echo "Invalid to_tag format. Expected X.Y.Z or X.Y.Z.W (numeric), got: ${TAG}" >> "$GITHUB_STEP_SUMMARY" | ||
| exit 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
The version format validation at line 83 allows a fourth component (X.Y.Z.W) but doesn't validate that each component is within reasonable bounds. Very large version numbers could cause issues downstream. Consider adding validation for the numeric range of version components.
| # Enforce reasonable numeric bounds on each version component to avoid overflow issues downstream | |
| MAX_VERSION_COMPONENT=2147483647 | |
| IFS='.' read -r -a VERSION_PARTS <<< "${TAG}" | |
| for part in "${VERSION_PARTS[@]}"; do | |
| if (( part < 0 || part > MAX_VERSION_COMPONENT )); then | |
| echo "Invalid to_tag component '${part}'. Each numeric component must be between 0 and ${MAX_VERSION_COMPONENT}." >> "$GITHUB_STEP_SUMMARY" | |
| exit 1 | |
| fi | |
| done |
| -H "X-GitHub-Api-Version: 2022-11-28" \ | ||
| -H "Accept: application/vnd.github+json" \ | ||
| -H "Authorization: Bearer ${RELEASE_TOKEN}" \ | ||
| -d "$(jq -n '{state:"closed"}')" >/dev/null |
There was a problem hiding this comment.
At line 862, the curl command uses 'jq -n' to construct JSON with a state field, but the value 'closed' is passed as a bare string which will be treated as a JSON string literal. While this appears to be correct, the inconsistency with how other API calls construct their JSON (using --arg for string values) could lead to confusion. Consider using consistent JSON construction patterns throughout.
| -d "$(jq -n '{state:"closed"}')" >/dev/null | |
| -d "$(jq -n --arg state "closed" '{state:$state}')" >/dev/null |
| fi | ||
| done | ||
|
|
||
| PAGE=$((PAGE+1)) |
There was a problem hiding this comment.
The HTTP status code checking at lines 730-732 limits pagination to 50 pages (5000 issues) for bugfix releases, while lines 828-832 use the same 50-page limit for minor releases. However, lines 573-575 and 622-624 use a 20-page limit for milestone searches. This inconsistency in pagination limits could lead to unpredictable behavior. Consider standardizing the pagination limits or documenting why different limits are used.
| PAGE=$((PAGE+1)) | |
| PAGE=$((PAGE+1)) | |
| # NOTE: This job intentionally allows up to 50 pages (5000 issues) when moving | |
| # bugfix milestone issues. Other milestone/search steps in this workflow may | |
| # use a lower page limit (e.g. 20 pages) for performance, but this higher | |
| # limit ensures large bugfix milestones are fully processed. |
| if [[ -z "${NEW_NUM}" || "${NEW_NUM}" == "null" ]]; then | ||
| echo "Failed to create milestone '${TITLE}'" >> "$GITHUB_STEP_SUMMARY" | ||
| echo "$(echo "${NEW_JSON}" | head -c 2000)" >> "$GITHUB_STEP_SUMMARY" | ||
| exit 1 |
There was a problem hiding this comment.
The error handling at lines 643-645 exits with status 1 when failing to create a milestone, which will fail the entire workflow. However, this is inside a loop that's trying to create multiple milestones. A failure to create one milestone will prevent the creation of subsequent milestones. Consider whether partial success should be allowed or if all-or-nothing is the desired behavior.
| exit 1 | |
| continue |
| echo "created_map=${CREATED}" >> "$GITHUB_OUTPUT" | ||
|
|
||
| - name: Move issues for bugfix (from old milestone to new milestone) | ||
| if: ${{ steps.plan.outputs.do_milestones == 'true' && steps.plan.outputs.move_from_title != '' && steps.plan.outputs.move_to_title != '' && env.RELEASE_KIND == 'bugfix' }} |
There was a problem hiding this comment.
The condition checking at line 655 uses a simple equality comparison for the RELEASE_KIND environment variable, but the RELEASE_KIND is passed from the previous job's output. If the previous job fails or is skipped, this could lead to unexpected behavior. Consider adding additional safety checks to ensure RELEASE_KIND is properly set before using it in conditions.
| if: ${{ steps.plan.outputs.do_milestones == 'true' && steps.plan.outputs.move_from_title != '' && steps.plan.outputs.move_to_title != '' && env.RELEASE_KIND == 'bugfix' }} | |
| if: ${{ steps.plan.outputs.do_milestones == 'true' && steps.plan.outputs.move_from_title != '' && steps.plan.outputs.move_to_title != '' && env.RELEASE_KIND != '' && env.RELEASE_KIND == 'bugfix' }} |
|
|
||
| CREATE_RELEASE=$(curl -v -w "\nHTTP_STATUS:%{http_code}" -X POST --location "https://api.github.com/repos/pimcore/$TARGET_REPO/releases" \ | ||
| RELEASE_URL=$(echo "${CREATE_RELEASE}" | sed '$d' | jq -r '.html_url // ""') | ||
| echo "Release created (draft): ${RELEASE_URL}" >> "$GITHUB_STEP_SUMMARY" |
There was a problem hiding this comment.
The release URL extraction at line 310 uses 'sed' and 'jq' to parse the response, but doesn't validate if the URL is empty or null before writing to the summary. If the API response is malformed, this could result in a confusing summary message. Consider adding validation to ensure RELEASE_URL is not empty before logging it.
| echo "Release created (draft): ${RELEASE_URL}" >> "$GITHUB_STEP_SUMMARY" | |
| if [[ -n "${RELEASE_URL}" ]]; then | |
| echo "Release created (draft): ${RELEASE_URL}" >> "$GITHUB_STEP_SUMMARY" | |
| else | |
| echo "Release created (draft), but release URL could not be determined from API response." >> "$GITHUB_STEP_SUMMARY" | |
| fi |
| permissions: | ||
| contents: write | ||
| needs: [select-release, milestone-automation] | ||
| if: ${{ !cancelled() && needs.select-release.result == 'success' && (needs.select-release.outputs.release_kind == 'major' || needs.select-release.outputs.release_kind == 'minor') }} |
There was a problem hiding this comment.
The branch-automation job at line 871 has a compound condition that checks if release_kind is 'major' or 'minor'. However, this condition is evaluated at the job level, which means if the select-release job fails but sets outputs, the condition might still evaluate incorrectly. Consider adding explicit null/empty checks for the release_kind output value.
| if: ${{ !cancelled() && needs.select-release.result == 'success' && (needs.select-release.outputs.release_kind == 'major' || needs.select-release.outputs.release_kind == 'minor') }} | |
| if: ${{ !cancelled() && needs.select-release.result == 'success' && needs.select-release.outputs.release_kind != '' && needs.select-release.outputs.release_kind != null && (needs.select-release.outputs.release_kind == 'major' || needs.select-release.outputs.release_kind == 'minor') }} |
| while true; do | ||
| RESP=$(curl -s --location \ | ||
| "https://api.github.com/repos/${OWNER}/${REPO}/milestones?state=all&per_page=100&page=${PAGE}" \ |
There was a problem hiding this comment.
no need to loop through all (open/closed) milestones, the direct get should be enough to check if it's FOUND or not
https://docs.github.com/en/rest/issues/milestones?apiVersion=2022-11-28#get-a-milestone--status-codes
with paging and going through all, it would be an overkill and slower
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if [[ "${BASE_IS_X}" == "true" ]]; then | ||
| if [[ "${TAG_MAJOR}" != "${BASE_MAJOR}" ]]; then | ||
| RELEASE_KIND="major" | ||
| else | ||
| RELEASE_KIND="minor" | ||
| fi | ||
| else | ||
| if [[ "${TAG_PARTS_COUNT}" -eq 4 ]]; then | ||
| RELEASE_KIND="hotfix" | ||
| elif [[ "${TAG_PARTS_COUNT}" -eq 3 ]]; then | ||
| RELEASE_KIND="bugfix" | ||
| else | ||
| echo "Could not classify release kind. base_branch=${BASE} to_tag=${TAG}" >> "$GITHUB_STEP_SUMMARY" | ||
| exit 1 | ||
| fi | ||
| fi |
There was a problem hiding this comment.
The validation regex allows versions like "12.4.0" or "12.4.0.1" (up to 4 numeric segments), but the release kind classification logic in lines 104-119 only handles 3 or 4 segments for non-X branches. However, there's no validation that ensures the tag format matches the expected branch format. For example, if base_branch is "12.x" but to_tag is "12.4.0.1" (4 segments), the code classifies it as "minor" on line 108, but hotfix releases (4 segments) shouldn't come from .x branches. This inconsistency could lead to incorrect release classification.
| echo "Please create release manually..." | ||
| exit 1 | ||
| # Parse TAG parts | ||
| IFS='.' read -r A B C D <<< "${TAG}" |
There was a problem hiding this comment.
The same variable 'D' is declared but never used here as well. Same issue as in the milestone-automation job - it's declared to handle 4-part version numbers but not referenced in this block. Consider using '_' for the unused variable for clarity.
| IFS='.' read -r A B C D <<< "${TAG}" | |
| IFS='.' read -r A B C _ <<< "${TAG}" |
| -H "Authorization: Bearer ${RELEASE_TOKEN}") | ||
| local LEN | ||
| LEN=$(echo "${RESP}" | jq '. | length') | ||
| if [[ "${LEN}" -eq 0 ]]; then | ||
| echo "" | ||
| return 0 | ||
| fi | ||
| local NUM | ||
| NUM=$(echo "${RESP}" | jq -r --arg t "${TITLE}" '.[] | select(.title == $t) | .number' | head -n 1) |
There was a problem hiding this comment.
Function variable declarations use 'local' keyword without explicit declaration. While 'local RESP' on line 500 works in bash, the pattern on lines 506 and 512 where variables are declared local and then assigned on the next line using command substitution could fail silently if the command fails. The 'local' command returns 0 even if the assignment fails. Consider combining declaration and assignment 'local LEN=$(...)' or checking command exit status separately to ensure errors are caught by 'set -e'.
| -H "Authorization: Bearer ${RELEASE_TOKEN}") | |
| local LEN | |
| LEN=$(echo "${RESP}" | jq '. | length') | |
| if [[ "${LEN}" -eq 0 ]]; then | |
| echo "" | |
| return 0 | |
| fi | |
| local NUM | |
| NUM=$(echo "${RESP}" | jq -r --arg t "${TITLE}" '.[] | select(.title == $t) | .number' | head -n 1) | |
| -H "Authorization: Bearer ${RELEASE_TOKEN}") || return 1 | |
| local LEN | |
| LEN=$(echo "${RESP}" | jq '. | length') || return 1 | |
| if [[ "${LEN}" -eq 0 ]]; then | |
| echo "" | |
| return 0 | |
| fi | |
| local NUM | |
| NUM=$(echo "${RESP}" | jq -r --arg t "${TITLE}" '.[] | select(.title == $t) | .number' | head -n 1) || return 1 |
| SHA_RESP=$(curl -s -w "\nHTTP_STATUS:%{http_code}" --location \ | ||
| "https://api.github.com/repos/${OWNER}/${REPO}/commits/${BASE}" \ | ||
| -H "X-GitHub-Api-Version: 2022-11-28" \ | ||
| -H "Accept: application/vnd.github+json" \ | ||
| -H "Authorization: Bearer ${RELEASE_TOKEN}") |
There was a problem hiding this comment.
The 'RELEASE_TOKEN' secret is accessed directly in the 'prepare' job step without proper error handling. If this secret is not set or is invalid, the curl command on line 123 will fail silently or provide unclear errors. Consider adding validation to check if RELEASE_TOKEN is set before making API calls, or at minimum improve the error message when the API call fails to indicate that it might be due to missing/invalid credentials.
|
|
||
| if [[ "${GEN_STATUS}" -eq 200 ]]; then | ||
| GEN_NOTES=$(echo "${GEN_RESPONSE}" | sed '$d' | jq -r '.body // ""') | ||
| FINAL_BODY="${GEN_NOTES}\n\n---\n\n${CHANGELOG}" |
There was a problem hiding this comment.
Potential newline interpretation issue in changelog concatenation. The '\n\n---\n\n' separator uses literal backslash-n characters rather than actual newlines. Depending on how this is interpreted by the GitHub API, this might result in literal '\n' strings appearing in the release notes instead of actual line breaks. Consider using printf or a heredoc to properly format multi-line strings with actual newlines.
| FINAL_BODY="${GEN_NOTES}\n\n---\n\n${CHANGELOG}" | |
| FINAL_BODY="$GEN_NOTES"$'\n\n---\n\n'"$CHANGELOG" |
| echo "move_from_title=${TAG}" >> "$GITHUB_OUTPUT" | ||
| echo "move_to_title=${NEXT_PATCH}" >> "$GITHUB_OUTPUT" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Missing opening 'else' statement. The 'fi' on this line closes an 'if' statement that was never opened at this level. Looking at the structure, line 349 opens an 'else' block after checking for hotfix, but line 372 has an extra 'fi' that doesn't match any 'if'. This will cause a shell syntax error when the workflow runs.
| permissions: | ||
| contents: write | ||
| needs: prepare | ||
| environment: ${{ inputs.publish_immediately && 'production' || '' }} |
There was a problem hiding this comment.
The environment protection is conditionally set using a ternary-like expression, but GitHub Actions doesn't support empty string as "no environment". When 'publish_immediately' is false, this evaluates to an empty string, which might not behave as intended. The workflow may either fail or create an environment with an empty name. Consider using a separate job or using 'if' conditions to control whether the production environment is required, rather than trying to conditionally set the environment name.
| environment: ${{ inputs.publish_immediately && 'production' || '' }} | |
| environment: production |
- Add validation to prevent 4-part versions from .x branches - Fix make_latest API parameter to use string type instead of boolean - Replace unused variable D with underscore - Fix newline handling in changelog concatenation - Remove conditional environment (not supported by GitHub Actions) - Improve error handling in resolve_ms function
| echo "⚠️ **Manual Action Required**: Rename branch '${FROM}' to '${TO}'" >> "$GITHUB_STEP_SUMMARY" | ||
| echo "" >> "$GITHUB_STEP_SUMMARY" | ||
| echo "After renaming, please check and update:" >> "$GITHUB_STEP_SUMMARY" | ||
| echo "- composer.json (branch-alias)" >> "$GITHUB_STEP_SUMMARY" | ||
| echo "- Workflow files (any hardcoded version references)" >> "$GITHUB_STEP_SUMMARY" | ||
| echo "- Documentation files" >> "$GITHUB_STEP_SUMMARY" | ||
| echo "" >> "$GITHUB_STEP_SUMMARY" | ||
| echo "See: https://github.com/${OWNER}/${REPO}/branches" >> "$GITHUB_STEP_SUMMARY" |
There was a problem hiding this comment.
it would be even better if this part of the workflow could do a partial git clone of the affected files (i will provide you the full list), a search and replace via sed in terminal then save the file, git add all, commit and push in the master branch
There was a problem hiding this comment.
in general master branch references in docs (blob/12.x/ in *.md), workflows and github templates
- https://github.com/pimcore/pimcore/blob/53b37fe10f7910a488f6936b68a05961cae317e4/.github/ISSUE_TEMPLATE/Bug-Report.yaml?plain=1#L14
- https://github.com/pimcore/pimcore/blob/12.x/.github/PULL_REQUEST_TEMPLATE.md?plain=1#L4-L5
- https://github.com/pimcore/pimcore/blob/0b14457ee4777a75d55e47569a9e3e8280ed588c/.github/workflows/pull-request-checklist.yaml#L19
- https://github.com/pimcore/pimcore/blob/0b14457ee4777a75d55e47569a9e3e8280ed588c/.github/workflows/codeception.yaml#L16
- https://github.com/pimcore/pimcore/blob/0b14457ee4777a75d55e47569a9e3e8280ed588c/.github/workflows/sync-changes-scheduled.yml#L14
- https://github.com/pimcore/pimcore/blob/0b14457ee4777a75d55e47569a9e3e8280ed588c/.github/workflows/composer-checks-outdated.yaml#L19-L20
- https://github.com/pimcore/pimcore/blob/0b14457ee4777a75d55e47569a9e3e8280ed588c/.github/workflows/composer-checks-vulnerabilities.yaml#L19-L20
- https://github.com/pimcore/pimcore/blob/6e1865e65944755d891cc5e1453571d62d1818f6/composer.json#L212
- https://github.com/pimcore/pimcore/blob/53b37fe10f7910a488f6936b68a05961cae317e4/CONTRIBUTING.md#L11
might be more, feel free to add if anyone notice i've missed anything
There was a problem hiding this comment.
The previous comment was restricted to pimcore/pimcore, for other bundles, might be trickier but
basically matrix.pimcore_version
- https://github.com/pimcore/workflows-collection-public/blob/main/.github/workflows/reusable-codeception-tests.yaml#L52
- https://github.com/pimcore/perspective-editor/blob/05f8f2465d25e7c67fedf23b842fd2562691eef9/.github/workflows/codeception.yml#L35 (not centralized workflow yet)
once core is tagged, go through all repos, mhh.. might be ad hoc step
There was a problem hiding this comment.
beware the search and replace must be not too greedy but more specific strings, eg. search for (12.3for bug fixes, others12.x) and replace with (13.0for bug fixes, others13.x), because there are places where 12.3, 12.x has to stay like that, eg. deprecations, changelog/upgrade notes, bc-layers, hardcoded comments
| if ! [[ "${BASE_MINOR}" =~ ^[0-9]+$ ]]; then | ||
| echo "Invalid base_branch format. Expected X.Y for maintenance branches, got: ${BASE}" >> "$GITHUB_STEP_SUMMARY" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
| if ! [[ "${BASE_MINOR}" =~ ^[0-9]+$ ]]; then | |
| echo "Invalid base_branch format. Expected X.Y for maintenance branches, got: ${BASE}" >> "$GITHUB_STEP_SUMMARY" | |
| exit 1 | |
| fi |
these more granular guards are not necessary here and redundant (and potentially slowing it down in terms of micro-optimization).
One preventive check at the beginning, like
https://github.com/pimcore/workflows-collection-public/pull/60/files#diff-a318819fb0ec8889018cbef8e6dd222e317757d95ddd93b62b714ac7c49f04f6R105-R108 but for ${BASE} would be enough.
There was a problem hiding this comment.
The only difference that i would do is to allow the v prefix to be added as user input (in case someone don't know it is not mandatory), and remove it from ${BASE} and have it automatically prefixed later when the v is necessary (for actual tag name), but it would be a nice-to-have: whoever is filling the input should be aware of it, or reminded by the input label.
| else | ||
| echo "⚠️ Warning: generate-notes failed (HTTP ${GEN_STATUS}); using custom notes only" >> "$GITHUB_STEP_SUMMARY" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Nice-to-have: maybe to "follow traditions", back in time when we didn't have generate release note function to rely on, we used to add the closed milestone as a link, see
https://github.com/pimcore/pimcore/releases/tag/v10.3.0
and we kept doing that for a while manually too (both generated and milestone link).
maybe it would be good to add it as a fixed suffix to the FINAL_BODY after a new line, so release notes would be technically never empty and have a link at "worst" case.
| else | ||
| echo "⚠️ Warning: generate-notes failed (HTTP ${GEN_STATUS}); using custom notes only" >> "$GITHUB_STEP_SUMMARY" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Nice-to-have: for EE bundles with a corresponding CE bundle, the generated release notes are like listing the PR of automated sync stuff.
eg. https://github.com/pimcore/ee-admin-ui-classic-bundle/releases/tag/v2.3.0
IIRC some partners complained in the past that this is not useful/meaningful, reason why i copy the same changelog from CE and paste it in EE, when i know there were no PRs for EE only.
https://github.com/pimcore/ee-admin-ui-classic-bundle/releases/tag/v2.2.2
If we could refine this workflow to perform the release on EE and CE at the same time and merge these release notes that could save some time.
More specifically, generate CE and EE release notes, store the CE one, go through the EE notes and remove everything starting with Automated PR by @pimcore-deployments in, prepend the CE notes (while removing Full Changelog: https://github.com/pimcore/ee-admin-ui-classic-bundle/compare/v2.2.0...v2.3.0).
The result would be the CE notes + any non-sync related PR in there.
There was a problem hiding this comment.
Might the CE-EE part not be this workflow, which is focused on single repo (iteration).
Maybe for the one that goes through the list of all repos+milestones in a csv format or so.
| if [[ -n "${FINAL_BODY}" ]]; then | ||
| REQ_BODY=$(jq -n \ | ||
| --arg target_commitish "${LATEST_SHA}" \ | ||
| --arg name "${RELEASE_NAME}" \ | ||
| --arg tag_name "${TAG_NAME}" \ | ||
| --arg make_latest "${MAKE_LATEST_STR}" \ | ||
| --argjson draft "${DRAFT}" \ | ||
| --argjson prerelease "${PRERELEASE}" \ | ||
| --arg body "${FINAL_BODY}" \ | ||
| '{ | ||
| target_commitish: $target_commitish, | ||
| name: $name, | ||
| draft: $draft, | ||
| make_latest: $make_latest, | ||
| prerelease: $prerelease, | ||
| tag_name: $tag_name, | ||
| generate_release_notes: false, | ||
| body: $body | ||
| }') | ||
| else | ||
| REQ_BODY=$(jq -n \ | ||
| --arg target_commitish "${LATEST_SHA}" \ | ||
| --arg name "${RELEASE_NAME}" \ | ||
| --arg tag_name "${TAG_NAME}" \ | ||
| --arg make_latest "${MAKE_LATEST_STR}" \ | ||
| --argjson draft "${DRAFT}" \ | ||
| --argjson prerelease "${PRERELEASE}" \ | ||
| --argjson generate_release_notes "${AUTO_CHANGELOG}" \ | ||
| '{ | ||
| target_commitish: $target_commitish, | ||
| name: $name, | ||
| draft: $draft, | ||
| make_latest: $make_latest, | ||
| prerelease: $prerelease, | ||
| tag_name: $tag_name, | ||
| generate_release_notes: $generate_release_notes | ||
| }') | ||
| fi |
There was a problem hiding this comment.
too much duplicated code, if the difference between these are 2 arguments/parameters
could it be refactored to share the identic code? reason is that any follow up on REQ_BODY base would require to edit in both cases and cause a micro-overhead.
| FOUND="" | ||
| while true; do | ||
| RESP=$(curl -s --location \ | ||
| "https://api.github.com/repos/${OWNER}/${REPO}/milestones?state=open&per_page=100&page=${PAGE}" \ |
- Strip leading v/V from to_tag input for flexibility - Auto-determine base branch from tag pattern (X.x for major/minor, X.Y for bugfix/hotfix) - Add upfront BASE validation (X.x or X.Y format) - Remove redundant BASE_MINOR validation check
- Introduce JQ_ARGS array with common arguments - Reduce code repetition in conditional REQ_BODY construction - Maintain target_commitish as BASE_BRANCH for branch-targeted releases
- Try creating milestone directly instead of paginating to check existence - Handle 422 (already exists) with single GET request to first page - Reduces API calls from 20+ pages to 1-2 calls per milestone - Eliminates unnecessary pagination loops
- Search open milestones first (most likely), then closed if not found - Add sort=created&direction=desc for better relevance - Limit to 3 pages per state (300 milestones max per state) - Reduces API calls from 20 pages (2000 milestones) to max 6 pages (600)
- Reuse created_map from milestone creation in all move steps - Reuse close_ms_number from find step in bugfix move (eliminates lookups) - Replace grep -oP with awk for robust created_map parsing - Extract title and number together with tab-separated jq output - Eliminates 40+ redundant API calls per release (85-90% reduction)
berfinyuksel
force-pushed
the
create_release_update
branch
from
28855bc to
0ab06a8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| TAG="${TAG}" | ||
| KIND="${RELEASE_KIND}" |
There was a problem hiding this comment.
Redundant variable assignments. The variables TAG and KIND are being assigned to themselves, which has no effect. These lines can be removed or if the intent was to copy environment variables to local shell variables, the assignment should reference the environment variables properly.
| if [[ "${KIND}" == "major" ]]; then | ||
| NEXT_MAJOR="$((A+1)).0.0" | ||
| NEXT_PATCH="${A}.${B}.$((C+1))" | ||
| echo "create_titles=${NEXT_MAJOR},${NEXT_PATCH}" >> "$GITHUB_OUTPUT" | ||
| if [[ "${A}" =~ ^[0-9]+$ && "${A}" -gt 0 ]]; then | ||
| PREV_MAJOR="$((A-1))." | ||
| echo "major_move_prefix=${PREV_MAJOR}" >> "$GITHUB_OUTPUT" | ||
| echo "move_to_title=${NEXT_PATCH}" >> "$GITHUB_OUTPUT" | ||
| fi | ||
| elif [[ "${KIND}" == "minor" ]]; then | ||
| NEXT_MINOR="${A}.$((B+1)).0" | ||
| NEXT_PATCH="${A}.${B}.$((C+1))" | ||
| echo "create_titles=${NEXT_MINOR},${NEXT_PATCH}" >> "$GITHUB_OUTPUT" | ||
| echo "move_to_title=${NEXT_PATCH}" >> "$GITHUB_OUTPUT" | ||
| if [[ "${B}" =~ ^[0-9]+$ && "${B}" -gt 0 ]]; then | ||
| PREV_MINOR="${A}.$((B-1))." | ||
| echo "minor_move_prefix=${PREV_MINOR}" >> "$GITHUB_OUTPUT" | ||
| fi | ||
| elif [[ "${KIND}" == "bugfix" ]]; then | ||
| NEXT_PATCH="${A}.${B}.$((C+1))" | ||
| NEXT_PATCH_2="${A}.${B}.$((C+2))" | ||
| echo "create_titles=${NEXT_PATCH},${NEXT_PATCH_2}" >> "$GITHUB_OUTPUT" | ||
| echo "move_from_title=${TAG}" >> "$GITHUB_OUTPUT" | ||
| echo "move_to_title=${NEXT_PATCH}" >> "$GITHUB_OUTPUT" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Incorrect indentation. The if block starting with "if [[ "${KIND}" == "major" ]];" should be indented to align with the surrounding code structure. The current indentation makes the code harder to read and violates standard bash scripting conventions.
| TAG="${TAG}" | ||
| KIND="${RELEASE_KIND}" |
There was a problem hiding this comment.
Redundant variable assignments. The variables TAG and KIND are being assigned to themselves, which has no effect. These lines can be removed or if the intent was to copy environment variables to local shell variables, the assignment should reference the environment variables properly.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 15 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Find milestone to close | ||
| id: ms_find | ||
| if: ${{ steps.plan.outputs.do_milestones == 'true' }} | ||
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
| TITLE="${{ steps.plan.outputs.close_title }}" | ||
|
|
||
| # Try open milestones first (most likely), then closed if not found | ||
| for STATE in open closed; do | ||
| PAGE=1 | ||
| MS_NUMBER="" | ||
| while [[ "${PAGE}" -le 3 ]]; do | ||
| RESP=$(curl -s --location \ | ||
| "https://api.github.com/repos/${OWNER}/${REPO}/milestones?state=${STATE}&sort=created&direction=desc&per_page=100&page=${PAGE}" \ | ||
| -H "X-GitHub-Api-Version: 2022-11-28" \ | ||
| -H "Accept: application/vnd.github+json" \ | ||
| -H "Authorization: Bearer ${RELEASE_TOKEN}") | ||
|
|
||
| LEN=$(echo "${RESP}" | jq '. | length') | ||
| if [[ "${LEN}" -eq 0 ]]; then | ||
| break | ||
| fi | ||
|
|
||
| MS_NUMBER=$(echo "${RESP}" | jq -r --arg t "${TITLE}" '.[] | select(.title == $t) | .number' | head -n 1) | ||
| if [[ -n "${MS_NUMBER}" && "${MS_NUMBER}" != "null" ]]; then | ||
| echo "close_ms_number=${MS_NUMBER}" >> "$GITHUB_OUTPUT" | ||
| echo "Found milestone '${TITLE}' as #${MS_NUMBER} (state=${STATE})" >> "$GITHUB_STEP_SUMMARY" | ||
| exit 0 | ||
| fi | ||
|
|
||
| PAGE=$((PAGE+1)) | ||
| done | ||
| done | ||
|
|
||
| echo "⚠️ Close milestone not found by title: ${TITLE} (searched 600 recent milestones; will not close)" >> "$GITHUB_STEP_SUMMARY" |
There was a problem hiding this comment.
The milestone search logic iterates through only the first 3 pages (300 milestones) for each state. If a repository has more than 600 total milestones, the target milestone might not be found even if it exists. The warning message mentions "searched 600 recent milestones" which is accurate, but consider if this limit is sufficient for repositories with many milestones. Alternatively, consider using the milestone title as a search parameter if the GitHub API supports it.
| select-release: | ||
| name: Select release outputs | ||
| runs-on: ubuntu-latest | ||
| needs: [create-release] | ||
| if: ${{ !cancelled() && needs.create-release.result == 'success' }} | ||
| outputs: | ||
| owner: ${{ needs.create-release.outputs.owner }} | ||
| repo: ${{ needs.create-release.outputs.repo }} | ||
| release_kind: ${{ needs.create-release.outputs.release_kind }} | ||
| tag: ${{ needs.create-release.outputs.tag }} | ||
| steps: | ||
| - name: Pass through outputs | ||
| shell: bash | ||
| run: | | ||
| echo "Release outputs passed through from create-release job" >> "$GITHUB_STEP_SUMMARY" | ||
|
|
||
| milestone-automation: | ||
| name: Milestone automation | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| issues: write | ||
| contents: read | ||
| needs: [select-release] | ||
| if: ${{ !cancelled() && needs.select-release.result == 'success' }} | ||
| env: | ||
| RELEASE_TOKEN: ${{ secrets.RELEASE_TOKEN }} | ||
| OWNER: ${{ needs.select-release.outputs.owner }} | ||
| REPO: ${{ needs.select-release.outputs.repo }} | ||
| RELEASE_KIND: ${{ needs.select-release.outputs.release_kind }} | ||
| TAG: ${{ needs.select-release.outputs.tag }} |
There was a problem hiding this comment.
The select-release job serves only as a pass-through for outputs from the create-release job. This intermediate job appears unnecessary and adds complexity. The outputs from create-release could be consumed directly by the downstream jobs (milestone-automation and branch-automation) without this intermediary step. Consider removing this job to simplify the workflow structure.
| select-release: | |
| name: Select release outputs | |
| runs-on: ubuntu-latest | |
| needs: [create-release] | |
| if: ${{ !cancelled() && needs.create-release.result == 'success' }} | |
| outputs: | |
| owner: ${{ needs.create-release.outputs.owner }} | |
| repo: ${{ needs.create-release.outputs.repo }} | |
| release_kind: ${{ needs.create-release.outputs.release_kind }} | |
| tag: ${{ needs.create-release.outputs.tag }} | |
| steps: | |
| - name: Pass through outputs | |
| shell: bash | |
| run: | | |
| echo "Release outputs passed through from create-release job" >> "$GITHUB_STEP_SUMMARY" | |
| milestone-automation: | |
| name: Milestone automation | |
| runs-on: ubuntu-latest | |
| permissions: | |
| issues: write | |
| contents: read | |
| needs: [select-release] | |
| if: ${{ !cancelled() && needs.select-release.result == 'success' }} | |
| env: | |
| RELEASE_TOKEN: ${{ secrets.RELEASE_TOKEN }} | |
| OWNER: ${{ needs.select-release.outputs.owner }} | |
| REPO: ${{ needs.select-release.outputs.repo }} | |
| RELEASE_KIND: ${{ needs.select-release.outputs.release_kind }} | |
| TAG: ${{ needs.select-release.outputs.tag }} | |
| milestone-automation: | |
| name: Milestone automation | |
| runs-on: ubuntu-latest | |
| permissions: | |
| issues: write | |
| contents: read | |
| needs: [create-release] | |
| if: ${{ !cancelled() && needs.create-release.result == 'success' }} | |
| env: | |
| RELEASE_TOKEN: ${{ secrets.RELEASE_TOKEN }} | |
| OWNER: ${{ needs.create-release.outputs.owner }} | |
| REPO: ${{ needs.create-release.outputs.repo }} | |
| RELEASE_KIND: ${{ needs.create-release.outputs.release_kind }} | |
| TAG: ${{ needs.create-release.outputs.tag }} |
| if [[ "${KIND}" == "major" ]]; then | ||
| if [[ "${A}" =~ ^[0-9]+$ && "${A}" -gt 0 ]]; then | ||
| PREV_MAJOR=$((A-1)) | ||
| CLONE_FROM="${PREV_MAJOR}.x" | ||
| CLONE_TO="${A}.0" | ||
| else | ||
| RELEASE_URL=$(echo "$CREATE_RELEASE" | awk -F'"' '/"html_url":/ {print $4; exit}') | ||
| echo "Release created successfully: $RELEASE_URL" >> $GITHUB_STEP_SUMMARY | ||
| echo "Cannot determine previous major version for tag ${TAG}" >> "$GITHUB_STEP_SUMMARY" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
The branch automation only runs for major and minor releases, but the logic doesn't handle the case where a major release is A.0.0 and A=1 (i.e., the first major version). In line 830-836, when A=1, PREV_MAJOR would be 0, and trying to clone from "0.x" branch might not be appropriate. Consider handling the edge case of the first major release (1.0.0) differently, or at least adding a validation to skip branch cloning if PREV_MAJOR=0.
| name: Release summary | ||
| runs-on: ubuntu-latest | ||
| needs: [prepare, create-release, select-release, milestone-automation, branch-automation] | ||
| if: always() |
There was a problem hiding this comment.
The permissions section is missing from the prepare and summary jobs. While these jobs might not need write permissions, it's a best practice to explicitly declare permissions for all jobs in a workflow to follow the principle of least privilege. Consider adding an explicit permissions: block to these jobs, even if it's just contents: read or permissions: {} if no permissions are needed.
| if: always() | |
| if: always() | |
| permissions: | |
| contents: read |
| exit 1 | ||
| fi | ||
| fi | ||
|
|
There was a problem hiding this comment.
The PRERELEASE variable is hardcoded to false at line 188, which means all releases created by this workflow will never be marked as prereleases. This seems inconsistent with the comprehensive release type detection logic (major, minor, bugfix, hotfix). If the workflow needs to support creating prereleases (e.g., alpha, beta, RC versions), this logic would need to be extended. If prereleases are intentionally not supported, consider adding a comment explaining this decision.
| # Pre-releases are intentionally not supported by this workflow. | |
| # If pre-release handling is required (e.g. alpha/beta/RC tags), | |
| # extend the logic above to detect such tags and set PRERELEASE accordingly. |
| - name: Close current milestone (after issue moves) | ||
| if: ${{ steps.plan.outputs.do_milestones == 'true' && steps.ms_find.outputs.close_ms_number != '' }} | ||
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
| MS_NUMBER="${{ steps.ms_find.outputs.close_ms_number }}" | ||
| curl -s --fail-with-body -X PATCH --location \ | ||
| "https://api.github.com/repos/${OWNER}/${REPO}/milestones/${MS_NUMBER}" \ | ||
| -H "X-GitHub-Api-Version: 2022-11-28" \ | ||
| -H "Accept: application/vnd.github+json" \ | ||
| -H "Authorization: Bearer $RELEASE_TOKEN" \ | ||
| -d "$REQ_BODY") | ||
| -H "Authorization: Bearer ${RELEASE_TOKEN}" \ | ||
| -d "$(jq -n '{state:"closed"}')" >/dev/null | ||
| echo "Closed milestone #${MS_NUMBER} (after moving issues)" >> "$GITHUB_STEP_SUMMARY" |
There was a problem hiding this comment.
The close milestone step uses --fail-with-body flag with curl but doesn't check the HTTP status code. If the PATCH request fails, the workflow will exit but won't provide detailed error information in the summary. Consider capturing the response and status code to provide better error messages, similar to how other API calls in the workflow handle errors.
| if [[ "${{ needs.select-release.result }}" == "success" ]]; then | ||
| echo "**Repository**: ${{ needs.select-release.outputs.owner }}/${{ needs.select-release.outputs.repo }}" >> "$GITHUB_STEP_SUMMARY" | ||
| echo "**Tag**: v${{ needs.select-release.outputs.tag }}" >> "$GITHUB_STEP_SUMMARY" | ||
| echo "**Release Type**: ${{ needs.select-release.outputs.release_kind }}" >> "$GITHUB_STEP_SUMMARY" | ||
| else | ||
| echo "**Repository**: ${{ needs.prepare.outputs.owner }}/${{ needs.prepare.outputs.repo }}" >> "$GITHUB_STEP_SUMMARY" | ||
| echo "**Tag**: ${{ needs.prepare.outputs.tag_name }}" >> "$GITHUB_STEP_SUMMARY" | ||
| echo "**Release Type**: ${{ needs.prepare.outputs.release_kind }}" >> "$GITHUB_STEP_SUMMARY" | ||
| fi |
There was a problem hiding this comment.
The job uses both needs.prepare.outputs.* and needs.select-release.outputs.* depending on whether select-release succeeded. However, this fallback logic might not work as expected. If select-release fails, its outputs will be undefined (not populated), but the condition needs.select-release.result == 'success' is only checked once at line 907. The else block at line 911 tries to use needs.prepare.outputs.tag_name and other prepare outputs, but prepare.outputs.tag_name exists while select-release would have tag (different names). This inconsistency could cause issues. Verify that the output names match what's actually available from each job.
| # Verify token is available | ||
| if [[ -z "${RELEASE_TOKEN}" ]]; then | ||
| echo "❌ RELEASE_TOKEN secret is not set or empty" >> "$GITHUB_STEP_SUMMARY" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Test token validity with a simple API call | ||
| TEST_RESP=$(curl -s -o /dev/null -w "%{http_code}" --location \ | ||
| "https://api.github.com/user" \ | ||
| -H "X-GitHub-Api-Version: 2022-11-28" \ | ||
| -H "Accept: application/vnd.github+json" \ | ||
| -H "Authorization: Bearer ${RELEASE_TOKEN}") | ||
|
|
||
| if [[ "${TEST_RESP}" -eq 401 ]]; then | ||
| echo "❌ RELEASE_TOKEN is invalid or expired (HTTP 401)" >> "$GITHUB_STEP_SUMMARY" | ||
| echo "Please ensure:" >> "$GITHUB_STEP_SUMMARY" | ||
| echo "1. The token has not expired" >> "$GITHUB_STEP_SUMMARY" | ||
| echo "2. The token has 'repo' scope for accessing repositories" >> "$GITHUB_STEP_SUMMARY" | ||
| echo "3. The token is a classic PAT or fine-grained token with repository access" >> "$GITHUB_STEP_SUMMARY" | ||
| exit 1 | ||
| elif [[ "${TEST_RESP}" -ne 200 ]]; then | ||
| echo "⚠️ Unexpected response from GitHub API when validating token (HTTP ${TEST_RESP})" >> "$GITHUB_STEP_SUMMARY" | ||
| fi |
There was a problem hiding this comment.
The workflow uses RELEASE_TOKEN secret extensively but doesn't validate its scope/permissions before performing operations. While there's a basic validity check at line 67-82, this only checks that the token is valid for authentication, not that it has the necessary permissions (repo access, milestone write, branch creation, etc.). Consider adding more specific permission checks early in the workflow, or at least improve error messages when operations fail due to insufficient permissions.
| MAKE_LATEST_STR="true" | ||
| if [[ "${MAKE_LATEST}" == "false" ]]; then | ||
| MAKE_LATEST_STR="false" |
There was a problem hiding this comment.
The make_latest field in the GitHub Releases API accepts a string with specific values: "true", "false", or "legacy". The current implementation correctly converts the boolean environment variable to a string. However, this conversion logic has a flaw: when MAKE_LATEST is anything other than exactly "false", it will be set to "true". This could mask issues if the variable is undefined or has an unexpected value. Consider using a more explicit check like if [[ "${MAKE_LATEST}" == "true" ]]; then MAKE_LATEST_STR="true"; else MAKE_LATEST_STR="false"; fi to ensure proper handling of all cases.
| MAKE_LATEST_STR="true" | |
| if [[ "${MAKE_LATEST}" == "false" ]]; then | |
| MAKE_LATEST_STR="false" | |
| MAKE_LATEST_STR="false" | |
| if [[ "${MAKE_LATEST}" == "true" ]]; then | |
| MAKE_LATEST_STR="true" |
|
|
||
| if [[ "${GEN_STATUS}" -eq 200 ]]; then | ||
| GEN_NOTES=$(echo "${GEN_RESPONSE}" | sed '$d' | jq -r '.body // ""') | ||
| FINAL_BODY="${GEN_NOTES}"$'\n\n---\n\n'"${CHANGELOG}" |
There was a problem hiding this comment.
When both auto-generated notes and custom changelog are provided, they are concatenated with the auto-generated notes first. The separator "---" is used between them. However, if the auto-generated notes are empty or the API returns an empty body, this will result in starting with a newline and separator followed by the custom notes, which may look awkward. Consider checking if GEN_NOTES is non-empty before concatenating.
| FINAL_BODY="${GEN_NOTES}"$'\n\n---\n\n'"${CHANGELOG}" | |
| if [[ -n "${GEN_NOTES}" ]]; then | |
| FINAL_BODY="${GEN_NOTES}"$'\n\n---\n\n'"${CHANGELOG}" | |
| else | |
| FINAL_BODY="${CHANGELOG}" | |
| fi |
- Restore base_branch input field (12.x or 12.3) - Fetch latest SHA from base branch during validation - Use LATEST_SHA as target_commitish for release creation - Add SHA to summary output for transparency - Simplify tag parsing (no 'v' prefix handling in input)
Consolidated "Move issues (minor)" and "Move issues (major)" steps into a single parameterized step "Move issues by prefix (major/minor)". This eliminates ~170 lines of duplicated code and reduces maintenance burden. Benefits: - Single source of truth for prefix-based issue movement logic - Dynamic prefix selection based on RELEASE_KIND variable - Reduced risk of inconsistent updates between major/minor workflows - Improved maintainability and readability
The summary job's fallback logic had a naming mismatch that would cause incorrect tag display. When select-release fails and the fallback uses prepare outputs, it was using 'tag_name' (which includes "v" prefix) and adding another "v", resulting in "vv12.4.0". Changed to use 'release_name' (without "v" prefix) and add "v" explicitly, ensuring both success and fallback paths display tags consistently.
…lestones - Add HTTP status validation for all issue/PR fetch API calls - Add JSON response type validation to ensure we get arrays - Add detailed logging showing how many items found per page - Add individual failure tracking for each item move operation - Improve error messages to distinguish between truly empty milestones vs API failures This fixes the silent failure where PR #388 wasn't detected in milestone 2.4.1 because the API call failed without proper error handling.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ! [[ "${TAG}" =~ ^[0-9]+\.[0-9]+\.[0-9]+(\.[0-9]+)?$ ]]; then | ||
| echo "Invalid to_tag format. Expected X.Y.Z or X.Y.Z.W (numeric), got: ${TAG}" >> "$GITHUB_STEP_SUMMARY" |
There was a problem hiding this comment.
The regex pattern for validating tag format allows numeric-only components but doesn't validate against invalid semantic versioning patterns such as leading zeros (e.g., "01.02.03"). While this may be intentional, consider adding validation to reject versions with leading zeros if they violate the project's versioning conventions.
| if ! [[ "${TAG}" =~ ^[0-9]+\.[0-9]+\.[0-9]+(\.[0-9]+)?$ ]]; then | |
| echo "Invalid to_tag format. Expected X.Y.Z or X.Y.Z.W (numeric), got: ${TAG}" >> "$GITHUB_STEP_SUMMARY" | |
| if ! [[ "${TAG}" =~ ^(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)(\.(0|[1-9][0-9]*))?$ ]]; then | |
| echo "Invalid to_tag format. Expected X.Y.Z or X.Y.Z.W (numeric, no leading zeros), got: ${TAG}" >> "$GITHUB_STEP_SUMMARY" |
| FOUND=false | ||
| for STATE in open closed; do | ||
| PAGE=1 | ||
| while [[ "${PAGE}" -le 3 ]]; do |
There was a problem hiding this comment.
The pagination logic stops after 3 pages (line 397), which limits milestone search to 300 items. If the repository has more than 300 milestones across open and closed states, the target milestone might not be found even if it exists. Consider increasing the page limit or adding a comment explaining why 3 pages is sufficient for this use case.
| while [[ "${PAGE}" -le 3 ]]; do | |
| # Allow searching through more pages to avoid missing milestones in large repositories. | |
| # With per_page=100 and MAX_MILESTONE_PAGES=100, we can scan up to 10,000 milestones per state. | |
| MAX_MILESTONE_PAGES=100 | |
| while [[ "${PAGE}" -le "${MAX_MILESTONE_PAGES}" ]]; do |
| # If not in created_map, search open milestones (should be there since just created) | ||
| if [[ -z "${TO_NUM}" ]]; then | ||
| RESP=$(curl -s --location \ | ||
| "https://api.github.com/repos/${OWNER}/${REPO}/milestones?state=open&sort=created&direction=desc&per_page=100" \ | ||
| -H "X-GitHub-Api-Version: 2022-11-28" \ | ||
| -H "Accept: application/vnd.github+json" \ | ||
| -H "Authorization: Bearer ${RELEASE_TOKEN}") | ||
| TO_NUM=$(echo "${RESP}" | jq -r --arg t "${TO_TITLE}" '.[] | select(.title == $t) | .number' | head -n 1) | ||
| fi | ||
|
|
||
| # Get FROM_NUM from validated milestone | ||
| FROM_NUM="${{ steps.ms_find.outputs.close_ms_number }}" | ||
|
|
||
| if [[ -z "${FROM_NUM}" || "${FROM_NUM}" == "null" || -z "${TO_NUM}" || "${TO_NUM}" == "null" ]]; then | ||
| echo "⚠️ Cannot move issues: milestone numbers not found (from='${FROM_TITLE}', to='${TO_TITLE}')" >> "$GITHUB_STEP_SUMMARY" | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
There's a potential race condition in the "Move issues for bugfix" step. If the milestone was just created in the previous step and issues are being fetched immediately, GitHub's API might not have fully indexed the new milestone yet. Consider adding a small delay (1-2 seconds) after milestone creation before attempting to query for issues, or add retry logic if the milestone isn't found.
| if [[ "${KIND}" == "major" ]]; then | ||
| if [[ "${A}" =~ ^[0-9]+$ && "${A}" -gt 0 ]]; then | ||
| PREV_MAJOR=$((A-1)) | ||
| CLONE_FROM="${PREV_MAJOR}.x" | ||
| CLONE_TO="${A}.0" | ||
| else | ||
| RELEASE_URL=$(echo "$CREATE_RELEASE" | awk -F'"' '/"html_url":/ {print $4; exit}') | ||
| echo "Release created successfully: $RELEASE_URL" >> $GITHUB_STEP_SUMMARY | ||
| echo "Cannot determine previous major version for tag ${TAG}" >> "$GITHUB_STEP_SUMMARY" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
In the branch automation logic for major releases, if the tag is "1.0.0", the calculated PREV_MAJOR would be 0, resulting in CLONE_FROM being "0.x". The code should validate that PREV_MAJOR is greater than or equal to a minimum version (e.g., 1) or handle the first major release as a special case where no branch cloning is needed.
| echo "❌ Failed to get SHA for branch '${FROM}'. HTTP ${SHA_STATUS}" >> "$GITHUB_STEP_SUMMARY" | ||
| echo "Branch '${FROM}' does not exist or is not accessible." >> "$GITHUB_STEP_SUMMARY" | ||
| exit 1 |
There was a problem hiding this comment.
The workflow creates branches for maintenance but doesn't verify if the source branch exists before attempting to create the new branch. If the CLONE_FROM branch doesn't exist, the operation will fail on lines 894-905. While the error is caught and reported, for the first major version release (e.g., 1.0.0 trying to clone from 0.x), this will always fail with a confusing error message. Consider adding a check to see if this is the first release and skip branch creation in that case.
| echo "❌ Failed to get SHA for branch '${FROM}'. HTTP ${SHA_STATUS}" >> "$GITHUB_STEP_SUMMARY" | |
| echo "Branch '${FROM}' does not exist or is not accessible." >> "$GITHUB_STEP_SUMMARY" | |
| exit 1 | |
| if [[ "${SHA_STATUS}" -eq 404 ]]; then | |
| echo "ℹ️ Source branch '${FROM}' not found (HTTP 404). Assuming this is the first release and skipping maintenance branch creation." >> "$GITHUB_STEP_SUMMARY" | |
| exit 0 | |
| else | |
| echo "❌ Failed to get SHA for branch '${FROM}'. HTTP ${SHA_STATUS}" >> "$GITHUB_STEP_SUMMARY" | |
| echo "Branch '${FROM}' does not exist or is not accessible." >> "$GITHUB_STEP_SUMMARY" | |
| exit 1 | |
| fi |
|
|
||
| if [[ "${GEN_STATUS}" -eq 200 ]]; then | ||
| GEN_NOTES=$(echo "${GEN_RESPONSE}" | sed '$d' | jq -r '.body // ""') | ||
| FINAL_BODY="${GEN_NOTES}"$'\n\n---\n\n'"${CHANGELOG}" |
There was a problem hiding this comment.
When both auto-generated notes and custom notes are requested, the auto-generated notes are placed BEFORE the custom notes (line 283). This ordering might be unexpected for users who provide custom notes intending them to be prominent. Consider documenting this behavior in the input description or reversing the order so custom notes appear first.
| # Search for milestone in open and closed states | ||
| FOUND=false | ||
| for STATE in open closed; do | ||
| PAGE=1 | ||
| while [[ "${PAGE}" -le 3 ]]; do | ||
| MS_RESP=$(curl -s -w "\nHTTP_STATUS:%{http_code}" --location \ | ||
| "https://api.github.com/repos/${OWNER}/${REPO}/milestones?state=${STATE}&sort=created&direction=desc&per_page=100&page=${PAGE}" \ | ||
| -H "X-GitHub-Api-Version: 2022-11-28" \ | ||
| -H "Accept: application/vnd.github+json" \ | ||
| -H "Authorization: Bearer ${RELEASE_TOKEN}") | ||
|
|
||
| MS_STATUS=$(echo "${MS_RESP}" | awk -F: '/HTTP_STATUS:/ {print $2}') | ||
| if [[ "${MS_STATUS}" -ne 200 ]]; then | ||
| echo "⚠️ Failed to fetch milestones (state=${STATE}, page=${PAGE}, HTTP ${MS_STATUS})" >> "$GITHUB_STEP_SUMMARY" | ||
| break | ||
| fi | ||
|
|
||
| RESP=$(echo "${MS_RESP}" | sed '$d') | ||
|
|
||
| if ! echo "${RESP}" | jq -e '. | type == "array"' > /dev/null 2>&1; then | ||
| echo "⚠️ Invalid API response (not a JSON array)" >> "$GITHUB_STEP_SUMMARY" | ||
| break | ||
| fi | ||
|
|
||
| if [[ $CHECK_TAG_NAME -eq 200 ]]; then | ||
| echo "Tag name already exists: $TAG_NAME" >> $GITHUB_STEP_SUMMARY | ||
| LEN=$(echo "${RESP}" | jq '. | length') | ||
| if [[ "${LEN}" -eq 0 ]]; then | ||
| break | ||
| fi | ||
|
|
||
| MS_NUMBER=$(echo "${RESP}" | jq -r --arg t "${TITLE}" '.[] | select(.title == $t) | .number' | head -n 1) | ||
| if [[ -n "${MS_NUMBER}" && "${MS_NUMBER}" != "null" ]]; then | ||
| echo "milestone_number=${MS_NUMBER}" >> "$GITHUB_OUTPUT" | ||
| echo "✅ Found milestone '${TITLE}' (#${MS_NUMBER}, state=${STATE})" >> "$GITHUB_STEP_SUMMARY" | ||
| FOUND=true | ||
| break 2 | ||
| fi | ||
|
|
||
| PAGE=$((PAGE+1)) | ||
| done | ||
| done |
There was a problem hiding this comment.
The milestone search iterates through both open and closed milestones, making multiple API calls (up to 6 total: 3 pages × 2 states). For repositories with many milestones, this could be slow and consume API rate limits. Consider optimizing by searching open milestones first (most likely case for new releases), and only searching closed milestones if not found in open ones.
When searching for milestones by prefix (e.g., '1.1.'), also match milestones with exact version without dot (e.g., '1.1'). This ensures milestone '1.1' is found when releasing 1.2.0 and moving its issues.
No description provided.