chore: handle test-lint-changelog failures differently for release branches #29612
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Builds ready [b354520]
Page Load Metrics (1657 ± 50 ms)
|
Builds ready [4d3d7c5]
Page Load Metrics (1630 ± 75 ms)
Bundle size diffs
|
Builds ready [e61eaf4]
Page Load Metrics (1737 ± 85 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Builds ready [af75307]
Page Load Metrics (1705 ± 64 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
chloeYue
changed the title
Builds ready [95e1850]
Page Load Metrics (1665 ± 45 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
| - name: Evaluate branch for changelog lint handling | ||
| id: evaluate-branch | ||
| run: | | ||
| BRANCH_NAME="${{ github.ref_name }}" |
There was a problem hiding this comment.
This should be an environment variable to the step, like:
env:
BRANCH_NAME=${{ ... }}
run: |
...There was a problem hiding this comment.
In addition, github.ref_name is only the branch name for the push and merge group events. For PRs, the name of the target branch is github.head_ref. github.ref_name for PRs returns something weird like refs/pull/3/merge.
There was a problem hiding this comment.
TLDR:
env:
# For a `pull_request` event, the branch is `github.head_ref``.
# For a `push` event, the branch is `github.ref_name`.
BRANCH: ${{ github.head_ref || github.ref_name }}| echo "Branch starts with Version-v. Ignoring failure for test-lint-changelog." | ||
| echo "IGNORE_CHANGELOG_FAILURE=true" >> "$GITHUB_ENV" | ||
| else | ||
| echo "IGNORE_CHANGELOG_FAILURE=false" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
it would be more idiomatic to make IGNORE_CHANGELOG_FAILURE the output of the step.
It would look something like:
echo "IGNORE_CHANGELOG_FAILURE=true" >> "$GITHUB_OUTPUT"
then you can later use it like:
steps.evaluate-branch.outputs.IGNORE_CHANGELOG_FAILURE
| - name: Validate test-lint-changelog outcome | ||
| id: validate-changelog-job | ||
| run: | | ||
| TEST_LINT_CHANGELOG_FAILED="${{ needs.test-lint-changelog.result }}" |
There was a problem hiding this comment.
same as above, TEST_LINT_CHANGELOG_FAILED and IGNORE_CHANGELOG_FAILURE should be passed in env, instead of being inlined.
There was a problem hiding this comment.
BTW, did you check if this works? I think that this will not work. As this job needs test-lint-changelog to succeed to even start running (as it is included in needs), this variable can never be equal failure.
There was a problem hiding this comment.
Use jobs.<job_id>.needs to identify any jobs that must complete successfully before this job will run. It can be a string or array of strings. If a job fails or is skipped, all jobs that need it are skipped.
| id: validate-changelog-job | ||
| run: | | ||
| TEST_LINT_CHANGELOG_FAILED="${{ needs.test-lint-changelog.result }}" | ||
| if [[ "$TEST_LINT_CHANGELOG_FAILED" == "failure" && "$IGNORE_CHANGELOG_FAILURE" == "true" ]]; then |
There was a problem hiding this comment.
I do not understand why is the same condition repeated twice. Why don't we just
if [[ "$TEST_LINT_CHANGELOG_FAILED" == "failure" ]]; then
if [[ "$IGNORE_CHANGELOG_FAILURE" == "true" ]]; then
echo "test-lint-changelog failed, but we're ignoring it for Version-v branches."
else
echo "test-lint-changelog failed and cannot be ignored."
exit 1
fi
fiI would even go as far as just doing:
if [[ "$TEST_LINT_CHANGELOG_FAILED" == "failure" ]]; then
if [[ "$BRANCH_NAME" == Version-v* ]]; then
echo "test-lint-changelog failed, but we're ignoring it for Version-v branches."
else
echo "test-lint-changelog failed and cannot be ignored."
exit 1
fi
fiso that we can reduce complexity by only having a single step. Having multiple steps for this is unnecessary in my opinion.
Description
This PR introduces conditional handling for the
test-lint-changelogjob based on the branch name in the pipeline.Specifically:
For release branches starting with
Version-v, failures in thetest-lint-changelogjob will:Be marked as failed for visibility.
Not block the pipeline, allowing subsequent jobs to execute.
For all other branches, failures in
test-lint-changelogwill:Block the pipeline.
Mark the workflow as failed, ensuring strict validation for non-RC branches.
The changes ensure that RC branches (starting with
Version-v) have flexibility in pipeline execution while maintaining visibility of validation failures. In other words, failure for jobtest-lint-changelogon release branch will not block the generation of builds.I have tested with the same change on a regular branch and a branch starts with
Version-v, it works as expected.Related issues
Fixes: #29721
Manual testing steps
Version-vand ensure:Failures in
test-lint-changelogdo not block the pipeline to generate builds but are still marked as failed in the UI.Version-vand ensure:Failures in
test-lint-changelogblock the pipeline to generate builds.Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist