Skip to content

fail-fast input setting#87

Open
bcomnes wants to merge 3 commits into
fastify:mainfrom
bcomnes:patch-1
Open

fail-fast input setting#87
bcomnes wants to merge 3 commits into
fastify:mainfrom
bcomnes:patch-1

Conversation

@bcomnes

@bcomnes bcomnes commented Jul 6, 2023

Copy link
Copy Markdown

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

Uzlopak commented Jul 6, 2023

Copy link
Copy Markdown
Contributor

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

@bcomnes

bcomnes commented Jul 6, 2023

Copy link
Copy Markdown
Author

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

Uzlopak commented Jul 6, 2023

Copy link
Copy Markdown
Contributor

Yes an input variable would be desired.

@bcomnes

bcomnes commented Jul 6, 2023

Copy link
Copy Markdown
Author

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
Comment thread .github/workflows/plugins-ci-mongo.yml Outdated
Uzlopak
Uzlopak previously approved these changes Jul 6, 2023

@Uzlopak Uzlopak left a comment

Copy link
Copy Markdown
Contributor

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

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@Fdawgs Fdawgs left a comment

Copy link
Copy Markdown
Member

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

bcomnes commented Jul 6, 2023

Copy link
Copy Markdown
Author

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

Uzlopak commented Jul 6, 2023

Copy link
Copy Markdown
Contributor

Is the workflow validation a false positive?

@bcomnes

bcomnes commented Jul 6, 2023

Copy link
Copy Markdown
Author

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

bcomnes commented Jul 6, 2023

Copy link
Copy Markdown
Author

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

bcomnes commented Jul 14, 2023

Copy link
Copy Markdown
Author

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

Fdawgs commented Jul 15, 2023

Copy link
Copy Markdown
Member

@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
Copy Markdown

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

bcomnes commented Jul 15, 2023

Copy link
Copy Markdown
Author

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

@Uzlopak

Uzlopak commented Jul 15, 2023

Copy link
Copy Markdown
Contributor

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

bcomnes commented Jul 16, 2023

Copy link
Copy Markdown
Author

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