Skip to content
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.

Sign up for GitHub

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

chore: handle test-lint-changelog failures differently for release branches #29612

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

chore: handle test-lint-changelog failures differently for release branches #29612

Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
b354520
test
chloeYue Jan 9, 2025
8f6a121
Merge branch 'main' into chloe-remove-rc-changelog-lint
chloeYue Jan 14, 2025
fe7f3ca
fix
chloeYue Jan 14, 2025
6304fce
revert change
chloeYue Jan 14, 2025
4aca5df
fix
chloeYue Jan 14, 2025
7e8c29c
test
chloeYue Jan 14, 2025
4d3d7c5
test
chloeYue Jan 14, 2025
f48ad9a
fix
chloeYue Jan 14, 2025
e61eaf4
test
chloeYue Jan 14, 2025
b28f763
test
chloeYue Jan 14, 2025
b77e846
revert circle ci yml
chloeYue Jan 14, 2025
e7cadff
test
chloeYue Jan 14, 2025
733ca4a
fix
chloeYue Jan 14, 2025
ecc3db1
fix
chloeYue Jan 14, 2025
cbf230b
fix
chloeYue Jan 14, 2025
af75307
test
chloeYue Jan 14, 2025
95e1850
improve
chloeYue Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,29 @@ jobs:
outputs:
PASSED: ${{ steps.set-output.outputs.PASSED }}
steps:
- name: Evaluate branch for changelog lint handling
id: evaluate-branch
run: |
BRANCH_NAME="${{ github.ref_name }}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be an environment variable to the step, like:

env:
  BRANCH_NAME=${{ ... }}
run: |
  ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }}

if [[ "$BRANCH_NAME" == Version-v* ]]; then
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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

fi

- name: Validate test-lint-changelog outcome
id: validate-changelog-job
run: |
TEST_LINT_CHANGELOG_FAILED="${{ needs.test-lint-changelog.result }}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above, TEST_LINT_CHANGELOG_FAILED and IGNORE_CHANGELOG_FAILURE should be passed in env, instead of being inlined.

Copy link
Contributor

@itsyoboieltr itsyoboieltr Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From: https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#jobsjob_idneeds

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.

if [[ "$TEST_LINT_CHANGELOG_FAILED" == "failure" && "$IGNORE_CHANGELOG_FAILURE" == "true" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
fi

I 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
fi

so that we can reduce complexity by only having a single step. Having multiple steps for this is unnecessary in my opinion.

echo "test-lint-changelog failed, but we're ignoring it for Version-v branches."
elif [[ "$TEST_LINT_CHANGELOG_FAILED" == "failure" ]]; then
echo "test-lint-changelog failed and cannot be ignored."
exit 1
fi
echo "All jobs completed successfully."

- name: Set PASSED output
id: set-output
run: echo "PASSED=true" >> "$GITHUB_OUTPUT"
Expand Down
Loading