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.

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: ci: fix if check #11639

Closed
wants to merge 9 commits into from
Closed

chore: ci: fix if check #11639

wants to merge 9 commits into from

Conversation

ArnaudBarre
Copy link
Member

@ArnaudBarre ArnaudBarre commented Jan 8, 2023

Re #11548
The condition did not work for #11638

This was way more complicated than I though. Really hard to make PR required checks behaves as expected when suing conditions.

Proposed solution:
See comment in the yml file. The issue with that is that is requires a transition period where require jobs will be off for old PRs. The advantage is that it will not happen later when we update node versions. This is the reason I opted for this solution.

Other solution:

  test-skip:
    needs: diff
    if: needs.diff.outputs.runTest == 'false'
    timeout-minutes: 1
    runs-on: ubuntu-latest
    strategy:
      matrix:
        os: [ubuntu-latest]
        node_version: [14, 16, 18]
        include:
          # Active LTS + other OS
          - os: macos-latest
            node_version: 18
          - os: windows-latest
            node_version: 18
    name: "Build&Test: node-${{ matrix.node_version }}, ${{ matrix.os }}"
    steps:
      - run: echo "No test required"

Other possibilities:

@ArnaudBarre ArnaudBarre marked this pull request as ready for review January 8, 2023 22:54
@dominikg
Copy link
Contributor

hmm, this is getting a lot more complex than i thought it would. how does this look like in the final PR ui? if it just shows required with a red or green check and people have to click through, that would be worse.

I have used a similar trick before with always and manual state checks, but would prefer if we didn't have to resort to this. turning off required and a transition period are also not great.

Are there other options like turning build and test commands into a noop instead of outright skipping the job? kind of like a mini nx, not going the lengths of complex dependency and inputs configs, just skipping if docs is the only changed thing

@ArnaudBarre
Copy link
Member Author

Yeah did that at the beginning and then though it would be cleaner to skip the job but yeah the final solution is worth that what i avoided at first. I will do that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants