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

fail-fast input setting #87

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

fail-fast input setting #87

wants to merge 3 commits into from

Conversation

bcomnes
Copy link

@bcomnes bcomnes commented Jul 6, 2023

When trying to debug CI errors in the matrix, its really frustrating to have to uncover them randomly one at a time. Can we change this so that all environments run to completion so its easier to find where problems live?

Checklist

@Uzlopak
Copy link
Contributor

Uzlopak commented Jul 6, 2023

You want to enforce this in fastify org? Or do you use our workflows for your repos, too?

@bcomnes
Copy link
Author

bcomnes commented Jul 6, 2023

I'm trying to fix CI on fastify-cli and an its really hard to tell which issue is a test problem and which is an environment problem since they keep failing in different orders. If they all ran to completion, it would be easier to determine this.

Perhaps this could be at a minimum, an input to the workflow so I could turn it on while debugging if fail-fast is actually the desired default.

fastify/fastify-cli#639

@Uzlopak
Copy link
Contributor

Uzlopak commented Jul 6, 2023

Yes an input variable would be desired.

@bcomnes
Copy link
Author

bcomnes commented Jul 6, 2023

Ok added a fail-fast input consistently across workflows that accept inputs.

@bcomnes bcomnes changed the title Dont fail-fast fail-fast input setting Jul 6, 2023
Uzlopak
Uzlopak previously approved these changes Jul 6, 2023
Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

LGTM

@Fdawgs wdyt?

mcollina
mcollina previously approved these changes Jul 6, 2023
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@Fdawgs Fdawgs left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Could you please fix the misspelling of 'strategy' in the descriptions added, and add the setting to the inputs table in the readme (alphabetically sorted)?

@bcomnes bcomnes dismissed stale reviews from mcollina and Uzlopak via 126c78a July 6, 2023 18:33
@bcomnes
Copy link
Author

bcomnes commented Jul 6, 2023

Thanks for the PR!

Could you please fix the misspelling of 'strategy' in the descriptions added, and add the setting to the inputs table in the readme (alphabetically sorted)?

Done.

@Uzlopak
Copy link
Contributor

Uzlopak commented Jul 6, 2023

Is the workflow validation a false positive?

@bcomnes
Copy link
Author

bcomnes commented Jul 6, 2023

Looks like its a boolean/string type coersion issue? Any advice? I tried following a similar pattern I saw elsewhere but I think if statements are special cases in actions.

@bcomnes bcomnes force-pushed the patch-1 branch 4 times, most recently from 05f967d to 126c78a Compare July 6, 2023 20:59
@bcomnes
Copy link
Author

bcomnes commented Jul 6, 2023

I dont understand, is it because the input is getting passed in as an expression that the validator is treating it not as a boolean? Is there a way to tell the validator that it's definitely a boolean?

@bcomnes
Copy link
Author

bcomnes commented Jul 14, 2023

Sorry I had to drop off this yak shave the other week.

Since it looks like there is a strange incompatibility between input type coercion and the validator tool here (happy to try and track that down and bring upstream if you want, please let me know if you do!), and I was able to isolate out the issues in the issues that prompted this PR, I have the following proposal:

Can anyone name a scenario where fail-fast: true is actually desired in the fastify org? The only case I can think of is if there were private repos burning paid minutes (which isn't a problem typically for open source projects). Otherwise, it seems like fail-fast: false should just be the default everywhere.

If folks agree, can we change it to that now, while we work through the upstream validator bugs?

@Fdawgs
Copy link
Member

Fdawgs commented Jul 15, 2023

@bcomnes could you rebase this PR please? Want to see if #88 has resolved this.

If not, tempted to just tear that action out. It's v0.x.x so not massively stable yet.

@voxpelli
Copy link

Can anyone name a scenario where fail-fast: true is actually desired in the fastify org? The only case I can think of is if there were private repos burning paid minutes (which isn't a problem typically for open source projects). Otherwise, it seems like fail-fast: false should just be the default everywhere.

I agree that in most cases fail-fast: false should be the desirable? (And it would be good to document why its not if there's a good reason for it, that way the likes of me won't comment on it)

@bcomnes
Copy link
Author

bcomnes commented Jul 15, 2023

I'll rebase the next time I'm at the keys.

@Uzlopak
Copy link
Contributor

Uzlopak commented Jul 15, 2023

I wonder why we dont have here the option to simply merge the default branch into this PR, as we can do it in fastify core repo.

bcomnes and others added 3 commits July 15, 2023 19:53
When trying to debug CI errors in the matrix, its really frustrating to have to uncover them randomly one at a time. Can we change this so that all environments run to completion so its easier to find where problems live?
@bcomnes
Copy link
Author

bcomnes commented Jul 16, 2023

Rebased.

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.

5 participants