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

Test bundle builds job before push #378

Merged
merged 5 commits into from
Oct 8, 2024

Conversation

FoamyGuy
Copy link
Contributor

Resolves: #377

This adds a new job to the bundle cron action which builds both the community and adafruit bundles.

The update-bundles job has had needs: test-bundle-builds added to it so that it depends upon the prior test builds job. If that test build fails then update-bundles will be skipped.

I tested this to the best of my abilities in a test repo. There are a few failed runs visible here: https://github.com/FoamyGuy/adabot_actions_tests/actions which validated the way that needs will skip the job if it's dependency failed. For testing I modified the task to add an erroneous extra file into one of the libraries similarly to what caused the recent build failures.

That being said, I'm not sure if there is a great way to test it within the cron task context without just merging it and checking in after the next time it runs. And if there is a desire to test the failure case we'd need to temporarily modify the test build action to make one or both of the builds fail. If anyone does know of a better way to test I'm happy to give it a try.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

I think you want to do a test build after the update happens but before it is commit and then released. This prevents a second bad build but not the first.

@FoamyGuy
Copy link
Contributor Author

FoamyGuy commented Oct 3, 2024

I am not sure that I understand. Do you mean handle this logic in python code after this line: https://github.com/adafruit/adabot/blob/main/adabot/circuitpython_bundle.py#L563 but before the following ones that commit and push it, rather than having the logic in an actions job only?

My understanding of the current implementation is that it won't allow a first bad build to be pushed because the task fetches all submodules recursively here: https://github.com/FoamyGuy/adabot/blob/test_build_before_push/.github/workflows/bundle_cron.yml#L41 before doing the test build. And then if the test build fails the entire update-bundles job will be skipped due to the added needs: test-bundle-builds. Which leads to adabot.circuitpython_bundle (https://github.com/FoamyGuy/adabot/blob/test_build_before_push/.github/workflows/bundle_cron.yml#L87) not being executed so no further building, nor committing or pushing will occur since they are inside of that.

I am still happy to change it up if there is a better way, or if I've misunderstood how the actions will behave. Maybe it's just nicer to keep it all in the python code rather than split between python and workflows.

@tannewt
Copy link
Member

tannewt commented Oct 4, 2024

I am not sure that I understand. Do you mean handle this logic in python code after this line: https://github.com/adafruit/adabot/blob/main/adabot/circuitpython_bundle.py#L563 but before the following ones that commit and push it, rather than having the logic in an actions job only?

Yes, I think that's what you need to do. The submodule recursive clone will set the submodules to the existing checked in state, not the updated state.

…anges from main. add test build bundle logic. remove unneeded if statement. allow RuntimeError to be raised.
# Conflicts:
#	adabot/circuitpython_bundle.py
@FoamyGuy
Copy link
Contributor Author

FoamyGuy commented Oct 5, 2024

The latest commits move the test build logic into the python code after the update and before the commit / push.

A few things about how it works now:

  • I changed it to re-raise the RuntimeError that was previously getting caught and consumed. That change is in order to make the actions task fail when there is a failure to build any bundle. Without this change the failed bundle wouldn't be commited or pushed, but the actions task would still succeed. I don't see any other code path that raises the Runtime error, but I only looked briefly and I'm not sure if this change could cause other issues during different operations that happen inside the try block.
  • Because the Adafruit bundle comes first in the list it means that if the Adafruit bundle builds successfully then it will get committed and pushed even if it turns out that the community bundle build fails.
  • I attempted to test as best as I can over in the branch: https://github.com/adafruit/adabot/tree/refs/heads/actions_bundlebuild_test. In that branch I removed the cron task and set it up for on push so that I could get it to run "manually". When both bundles build successfully the task succeeds. When a bundle fails to build the task fails, here is one such failure log: https://github.com/adafruit/adabot/actions/runs/11194745798 I made it fail by manipulating one of the libraries after the bundle updates and before the test build.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thanks! This looks more correct to me. One doc change. Good otherwise.

adabot/circuitpython_bundle.py Outdated Show resolved Hide resolved
@jepler
Copy link
Member

jepler commented Oct 8, 2024

Python documentation (pydoc) for sys.exit says the numeric argument is used as the "system exit status", and the docs.python.org documentation for SystemExit uses the same phrase. So if you're looking for an official-ese word for this, that's what I'd go with.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

This is better! Thank you!

@tannewt tannewt merged commit 6705480 into adafruit:main Oct 8, 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.

circuitpython_bundle: Check that bundle still builds before pushing
3 participants