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

Remove fail-fast matrix strategy from GitHub Actions #5421

Merged
merged 1 commit into from
Jun 21, 2023

Conversation

clayburn
Copy link
Contributor

Description

Currently, every GitHub Actions workflow contains the fail-fast matrix strategy. However, none of these workflows are actually utilizing a matrix. Further, the on demand workflow has an improper indentation of this strategy block that causes a syntactical error.

This change remove the no-op strategy altogether to fix the broken on demand workflow and remove the IDE warnings regarding the missing matrix declaration.

Related issue and scope

  • I opened an issue to propose and discuss this change (#????)

My changes affect the following components

  • API
  • Controller
  • Message Bus (e.g., Kafka)
  • Loadbalancer
  • Scheduler
  • Invoker
  • Intrinsic actions (e.g., sequences, conductors)
  • Data stores (e.g., CouchDB)
  • Tests
  • Deployment
  • CLI
  • General tooling
  • Documentation

Types of changes

  • Bug fix (generally a non-breaking change which closes an issue).
  • Enhancement or new feature (adds new functionality).
  • Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

  • I signed an Apache CLA.
  • I reviewed the style guides and followed the recommendations (Travis CI will check :).
  • I added tests to cover my changes.
  • My changes require further changes to the documentation.
  • I updated the documentation where necessary.

Currently, every GitHub Actions workflow contains the `fail-fast` matrix strategy. However, none of these workflows are actually utilizing a matrix. Further, the on demand workflow has an improper indentation of this strategy block that causes a syntactical error.

This change remove the no-op strategy altogether to fix the broken on demand workflow and remove the IDE warnings regarding the missing matrix declaration.
Copy link
Member

@style95 style95 left a comment

Choose a reason for hiding this comment

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

@clayburn
Thank you for your contribution.
I have tested workflows fails on any error here.
#5419 (comment)

And we are utilizing the fail-fast against the job strategy.
https://docs.github.com/ko/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstrategyfail-fast

What does it mean that none of these workflows are actually utilizing a matrix.?

@@ -65,8 +65,6 @@ jobs:
openwhisk:
runs-on: ubuntu-22.04
continue-on-error: false
strategy:
Copy link
Member

Choose a reason for hiding this comment

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

I was trying to fix this in this PR: https://github.com/apache/openwhisk/pull/5420/files

@codecov-commenter
Copy link

codecov-commenter commented Jun 21, 2023

Codecov Report

Merging #5421 (e643007) into master (88156c3) will decrease coverage by 0.48%.
The diff coverage is n/a.

❗ Current head e643007 differs from pull request most recent head f657a87. Consider uploading reports for the commit f657a87 to get more accurate results

@@            Coverage Diff             @@
##           master    #5421      +/-   ##
==========================================
- Coverage   76.84%   76.37%   -0.48%     
==========================================
  Files         241      241              
  Lines       14630    14630              
  Branches      616      616              
==========================================
- Hits        11243    11173      -70     
- Misses       3387     3457      +70     

see 16 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@clayburn
Copy link
Contributor Author

From the linked documentation on fail-fast:

jobs.<job_id>.strategy.fail-fast applies to the entire matrix. If jobs.<job_id>.strategy.fail-fast is set to true, GitHub will cancel all in-progress and queued jobs in the matrix if any job in the matrix fails. This property defaults to true.

This strategy block is specifically used to create a matrix strategy in order to run a job multiple times with different parameters. For example, you may do something like this to run a job against multiple operating systems:

strategy:
  fail-fast: true
  matrix:
    os: [ubuntu-latest, windows-latest]

In this case, if say ubuntu-latest were to fail, then windows would be cancelled. But since you are not using a matrix strategy like this, the fail-fast would be a no-op.

I also get this warning in my IDE, although I do not know where to locate the schema it is using for this validation:

Screenshot 2023-06-21 at 8 09 55 AM

Copy link
Member

@dgrove-oss dgrove-oss left a comment

Choose a reason for hiding this comment

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

LGTM

@dgrove-oss dgrove-oss merged commit f73ec7f into apache:master Jun 21, 2023
5 of 6 checks passed
@style95
Copy link
Member

style95 commented Jun 22, 2023

@clayburn Got it.
Thank you for the explanation.
I confirmed a build with the current master immediately stops on an error.
#5423

mtt-merz pushed a commit to mtt-merz/openwhisk that referenced this pull request Oct 22, 2023
Currently, every GitHub Actions workflow contains the `fail-fast` matrix strategy. However, none of these workflows are actually utilizing a matrix. Further, the on demand workflow has an improper indentation of this strategy block that causes a syntactical error.

This change remove the no-op strategy altogether to fix the broken on demand workflow and remove the IDE warnings regarding the missing matrix declaration.

(cherry picked from commit f73ec7f)
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

4 participants