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 automatic building in favor of just testing build completion #30

Merged
merged 5 commits into from
Feb 26, 2024

Conversation

bmos
Copy link
Contributor

@bmos bmos commented Feb 20, 2024

The build/test action was failing for new PRs and new releases (since these both target commits rather than branches). It was also doing un-needed extra work by submitting the PR three times (because of the multi-platform strategy). Sorry I didn't catch this in my prior commit.

@mavrosxristoforos
Copy link
Owner

Is this ready for review?

too many downsides to having this run on PRs and if it doesn't then there's no validation of submissions.
@bmos
Copy link
Contributor Author

bmos commented Feb 21, 2024

Is is now! It's now pretty much like the old action but targets node20.
If the idea of having it auto-build and create a PR when you commit changes that should be its own file with more limited triggering (as in this action at 23c9df2 but without the first job).

@bmos bmos changed the title only run build/test action on branches remove automatic building in favor of just testing build completion Feb 22, 2024
@mavrosxristoforos
Copy link
Owner

So, does it create a PR for auto-building now?

@bmos
Copy link
Contributor Author

bmos commented Feb 22, 2024

No, although I'm willing to rework this to retain that feature if you'd prefer.

@mavrosxristoforos
Copy link
Owner

So, for future contributors, would we need to build before committing?
I'm ok with both options. Does it create any PRs now? If yes, what for?

@bmos
Copy link
Contributor Author

bmos commented Feb 22, 2024

It does not create a PR at all.
It only runs the build process to ensure it completes without errors.

@mavrosxristoforos
Copy link
Owner

I think it's best this way...

@mavrosxristoforos mavrosxristoforos merged commit 692f2e6 into mavrosxristoforos:master Feb 26, 2024
3 checks passed
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.

2 participants