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

Simplify Matrix decode, add defaults for fail-fast and max-parallel, add test #763

Merged
merged 3 commits into from
Aug 9, 2021

Conversation

catthehacker
Copy link
Member

@catthehacker catthehacker commented Jul 26, 2021

fix[workflow]: default max-parallel
fix[workflow]: default fail-fast, it's true, not false
fix[workflow]: skipping over the job when strategy: is defined but matrix: isn't (fixes #625)
fix[workflow]: skip non-existing includes keys and hard fail on non-existing excludes keys
fix[workflow]: simplify Matrix decode (because I "think" I know how yaml works) (fixes #760)
fix[tests]: add test for planner and runner

@catthehacker catthehacker requested a review from a team as a code owner July 26, 2021 21:35
@mergify

This comment has been minimized.

@mergify mergify bot added the conflict PR has conflicts label Jul 26, 2021
@mergify mergify bot removed the conflict PR has conflicts label Jul 26, 2021
@codecov
Copy link

codecov bot commented Jul 26, 2021

Codecov Report

Merging #763 (e3eb7df) into master (0f04942) will increase coverage by 4.22%.
The diff coverage is 63.08%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #763      +/-   ##
==========================================
+ Coverage   49.27%   53.49%   +4.22%     
==========================================
  Files          23       23              
  Lines        2401     2658     +257     
==========================================
+ Hits         1183     1422     +239     
+ Misses       1090     1087       -3     
- Partials      128      149      +21     
Impacted Files Coverage Δ
pkg/container/docker_build.go 0.00% <0.00%> (ø)
pkg/container/docker_run.go 1.83% <0.00%> (-0.10%) ⬇️
pkg/common/git.go 52.85% <30.15%> (-6.94%) ⬇️
pkg/runner/runner.go 68.96% <33.33%> (-7.51%) ⬇️
pkg/model/planner.go 51.85% <41.37%> (+18.76%) ⬆️
pkg/model/workflow.go 51.08% <62.68%> (+25.37%) ⬆️
pkg/container/docker_pull.go 36.17% <64.70%> (+17.98%) ⬆️
pkg/runner/expression.go 85.38% <71.87%> (-1.26%) ⬇️
pkg/runner/step_context.go 75.00% <76.34%> (+6.05%) ⬆️
pkg/runner/run_context.go 80.36% <98.55%> (+3.95%) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43d46aa...e3eb7df. Read the comment docs.

@catthehacker catthehacker changed the title fix[workflow]: fix decode and align with GHA Simplify Matrix decode, add defaults for fail-fast and max-parallel, add test Jul 26, 2021
@mergify

This comment has been minimized.

@mergify mergify bot added the needs-work Extra attention is needed label Jul 26, 2021
@catthehacker

This comment has been minimized.

@mergify

This comment has been minimized.

@catthehacker

This comment has been minimized.

@catthehacker catthehacker self-assigned this Jul 27, 2021
@catthehacker catthehacker marked this pull request as draft July 27, 2021 16:10
@mergify

This comment has been minimized.

@catthehacker catthehacker marked this pull request as ready for review July 27, 2021 16:33
@mergify mergify bot removed the needs-work Extra attention is needed label Jul 27, 2021
@acjh
Copy link

acjh commented Jul 28, 2021

fix[workflow]: ... hard fail on non-existing excludes

// ... because that's what GitHub does for non-existing matrix keys, fail on exclude, ...

GitHub doesn't appear to do that.
e.g. speedy-net/speedy-net/actions/runs/1025416111/workflow (Test only Python 3.6 and 3.7 on non-master/staging)

strategy:
  matrix:
    python-version: [3.6, 3.7, 3.8, 3.9]
    is-master-or-staging:
      - ${{ github.ref == 'refs/heads/master' || github.ref == 'refs/heads/staging' }}
    exclude:
      - python-version: 3.8
        is-master-or-staging: false
      - python-version: 3.9
        is-master-or-staging: false

@catthehacker

This comment has been minimized.

@acjh
Copy link

acjh commented Jul 28, 2021

Could you share a link to that workflow and/or your strategy section?

@catthehacker

This comment has been minimized.

@catthehacker

This comment has been minimized.

@catthehacker
Copy link
Member Author

fix[workflow]: ... hard fail on non-existing excludes

// ... because that's what GitHub does for non-existing matrix keys, fail on exclude, ...

GitHub doesn't appear to do that.
e.g. speedy-net/speedy-net/actions/runs/1025416111/workflow (Test only Python 3.6 and 3.7 on non-master/staging)

strategy:
  matrix:
    python-version: [3.6, 3.7, 3.8, 3.9]
    is-master-or-staging:
      - ${{ github.ref == 'refs/heads/master' || github.ref == 'refs/heads/staging' }}
    exclude:
      - python-version: 3.8
        is-master-or-staging: false
      - python-version: 3.9
        is-master-or-staging: false

Your example has defined all required keys in matrix so that's why it's not failing

@acjh
Copy link

acjh commented Jul 28, 2021

Ah, I misunderstood "non-existing excludes" as "excludes not existing in the cross product".
Instead, you mean "excludes with any key not existing in the matrix".

Thanks for the clarification and your good work!

fix[workflow]: default `max-parallel`
fix[workflow]: default `fail-fast`, it's `true`, not `false`
fix[workflow]: skipping over the job when `strategy:` is defined but `matrix:` isn't (fixes nektos#625)
fix[workflow]: skip non-existing includes keys and hard fail on non-existing excludes keys
fix[workflow]: simplify Matrix decode (because I "think" I know how `yaml` works) (fixes nektos#760)
fix[tests]: add test for planner and runner
@catthehacker
Copy link
Member Author

updated commit description to mention that it's for keys

@mergify

This comment has been minimized.

@mergify mergify bot added the needs-work Extra attention is needed label Jul 28, 2021
@mergify mergify bot removed the needs-work Extra attention is needed label Jul 31, 2021
@catthehacker catthehacker added the stale-exempt Exempt from stale label Aug 8, 2021
@catthehacker catthehacker requested a review from a team August 8, 2021 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment