Skip to content

ci: 241 release pipeline does not depend on tests #257

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.

Sign up for GitHub

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

Jump to bottom
Merged
dan-fernandes merged 51 commits into from
Jun 12, 2025
Merged

ci: 241 release pipeline does not depend on tests #257

dan-fernandes merged 51 commits into from
Jun 12, 2025

Conversation

@dan-fernandes
Copy link
Contributor

@dan-fernandes dan-fernandes commented Apr 15, 2025 •
edited by DiamondJoseph
Loading

ACQP-380

Closes #241

dan-fernandes and others added 25 commits April 10, 2025 13:33
@dan-fernandes
Make pypi, release depend on test
93d1b55
@dan-fernandes
Split container build and push into two jobs
b7f2a2c
@dan-fernandes
Add call to _push_container.yml in ci.yml.jinja
55901ed
@dan-fernandes
Move _push_container.yml out of template, add symlink to new location
fcdb8b2
@dan-fernandes
Fix key name, fix file path
39adf2c
@dan-fernandes
Add ref_type=tag condition to push container
7cdac25
@dan-fernandes
Seperate _docs into _push_docs, add symlink
2cdd018
@dan-fernandes
Add repo checkout to _push_docs.yml
f262aa5
@dan-fernandes
Remove duplicate code
7a7a0a6
@dan-fernandes
Add _push_docs.yml to ci and tempalte ci
51ae06a
@dan-fernandes
Remove uneccesary wait
9168714
@dan-fernandes
Fix referenced SHA
78aa8bd
@dan-fernandes
Merge 241-release-pipeline-does-not-depend-on-tests branch from fork (#…
9ff5d03 
…256)

I started developing into a fork of this repo but tests don't pass due
to how the example repo is set up. Because of this I need to merge my
changes into a branch on the real repo.
@dan-fernandes
Have _push_docs call depend on test
307414e
@dan-fernandes
Make release depend on test
c2060f7
@dan-fernandes
Rename _push_docs job to publish
7894539
@dan-fernandes
Move _push_docs condition into ci
6d9dff8
@dan-fernandes
Add Load Image step
80178a8
@dan-fernandes
Remove load from build-push-action
816687c
@dan-fernandes
Remove context
0788359
@dan-fernandes
Make both _container.yml and _push_container.yml build and test conta…
ad03418 
…iner
@dan-fernandes
Make both _docs.yml and _push_docs.yml build docs
965f51f
@dan-fernandes
Re-add removed load attribute
67b49d7
@dan-fernandes
Remove output parameter to container build
0d09bb2
@dan-fernandes
Remove document artifact upload in _push_docs to avoid duplicate arti…
85d1671 
…fact
@dan-fernandes
Copy link
Contributor Author

dan-fernandes commented May 1, 2025 •
edited
Loading

The new CI appears to work correctly, apart from an issue releasing artifacts to GitHub. This is likely due to a known error in the workflow used, as mentioned in this thread.

We currently use the softprops/action-gh-release@7b4da11513bf3f43f9999e90eabced41ab8bb048 (v2.2.0) workflow to release artifacts.

In a test repo, a release using this exact commit failed.

When changed to softprops/action-gh-release@v2 (more recent than v2.2.0) the release succeeded.

In order to avoid the error we may need to change update the version of this workflow used in our CI.

@dan-fernandes
Copy link
Contributor Author

dan-fernandes commented May 1, 2025

I am also not sure that the names of each workflow are still clear. Please feel free to suggest name changes.

@dan-fernandes dan-fernandes changed the title 241 release pipeline does not depend on tests ci: 241 release pipeline does not depend on tests May 8, 2025
coretl
coretl reviewed May 9, 2025
View reviewed changes
Copy link
Contributor

@coretl coretl left a comment

Choose a reason for hiding this comment

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

Just one question, then I think this is ok to merge

@DiamondJoseph DiamondJoseph mentioned this pull request May 14, 2025
Closed
@coretl
Copy link
Contributor

coretl commented May 22, 2025

I would like to revisit the need for this in light of the upcoming uv changes, the lockfile should mean that we don't ever get into a situation where a merge to main passes tests in CI but a release fails tests

To discuss with @callumforrester when he's back

@callumforrester
Copy link
Contributor

callumforrester commented May 28, 2025

@coretl We have no specific timetable for merging the uv changes, just "when we trust them", could be 2 weeks or 6 months. I would suggest this change is still useful in the interim and if (not when) uv goes in we can consider reverting. This change doesn't add lots of complexity any more so the cost seems low to me.

coretl
coretl previously approved these changes Jun 9, 2025
View reviewed changes
@callumforrester
Copy link
Contributor

callumforrester commented Jun 9, 2025

@dan-fernandes Feel free to merge this, but if you have time it might be worth removing the dependency between docs and tests so they can run in parallel

callumforrester
callumforrester approved these changes Jun 12, 2025
View reviewed changes
Copy link
Contributor

@callumforrester callumforrester left a comment

Choose a reason for hiding this comment

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

Re-approving

@dan-fernandes dan-fernandes merged commit a7b863a into main Jun 12, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coretl coretl coretl left review comments

@GDYendell GDYendell Awaiting requested review from GDYendell

@DiamondJoseph DiamondJoseph Awaiting requested review from DiamondJoseph

+1 more reviewer

@callumforrester callumforrester callumforrester approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Release pipeline does not depend on tests

5 participants

@dan-fernandes @coretl @callumforrester @DiamondJoseph