Skip to content

Conversation

@jkislin
Copy link
Contributor

@jkislin jkislin commented Sep 4, 2025

Running tests. Will request review. It probably makes sense to leave the cache code commented in the script as we may change our mind again!

@codecov
Copy link

codecov bot commented Sep 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 42.37%. Comparing base (46529b9) to head (eec3176).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #659   +/-   ##
=======================================
  Coverage   42.37%   42.37%           
=======================================
  Files          30       30           
  Lines        2834     2834           
=======================================
  Hits         1201     1201           
  Misses       1633     1633           
Flag Coverage Δ
hewr 37.97% <ø> (ø)
pipelines 37.33% <ø> (ø)
pyrenew_hew 62.29% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jkislin
Copy link
Contributor Author

jkislin commented Sep 4, 2025

@dylanhmorris not sure what's going on with the format-suggest check, but otherwise, I removed the cacheing code and the container successfully builds and post to ghcr.io.

Let me know if there's a way I can help test the functionality of the container, or if that's even necessary given our automated tests. If not necessary, please feel free to review or provide feedback!

Thanks,
Jon

@dylanhmorris
Copy link
Contributor

dylanhmorris commented Sep 4, 2025

Let me know if there's a way I can help test the functionality of the container, or if that's even necessary given our automated tests. If not necessary, please feel free to review or provide feedback!

Thanks, @jkislin! I think the best way to test functionality before merging to try running a job in Azure with the jk-rm-cache version of the container via setup_job.py. Could you try that and report the results?

@jkislin
Copy link
Contributor Author

jkislin commented Sep 15, 2025

Let me know if there's a way I can help test the functionality of the container, or if that's even necessary given our automated tests. If not necessary, please feel free to review or provide feedback!

Thanks, @jkislin! I think the best way to test functionality before merging to try running a job in Azure with the jk-rm-cache version of the container via setup_job.py. Could you try that and report the results?

I am still getting a Manifest unknown error.
image

I will investigate more tomorrow.

@dylanhmorris
Copy link
Contributor

@jkislin I think with these simplifications we can just remove the docker_build_and_push.sh script and use the official action, no? https://github.com/docker/build-push-action

Might resolve the manifest issues and should in general be more robust

@damonbayer
Copy link
Collaborator

@jkislin I think with these simplifications we can just remove the docker_build_and_push.sh script and use the official action, no? docker/build-push-action

Might resolve the manifest issues and should in general be more robust

@jkislin What do you think of this?

@jkislin
Copy link
Contributor Author

jkislin commented Dec 16, 2025

@jkislin I think with these simplifications we can just remove the docker_build_and_push.sh script and use the official action, no? docker/build-push-action
Might resolve the manifest issues and should in general be more robust

@jkislin What do you think of this?

It's possible that would help, I am noticing that there's either inappropriate (or we just don't understand exactly) redirect behavior when specifying image tags on ghcr.io.

image

Any tag I supply directs here. There's a "latest" meta-tag that applies even if the image pushed wasn't tagged "latest".

I'm now also getting this error on the pool itself. I'm going to see if making "latest" the actual "latest" changes things.

@jkislin jkislin changed the title Removing container build cacheing to assess build reliability without it Refactoring the container build process in Github Actions for improved reliability Dec 16, 2025
@jkislin
Copy link
Contributor Author

jkislin commented Dec 16, 2025

I've refactored for the build-and-push action, and we can toggle cacheing via commenting/uncommenting the final two lines.

Copy link
Contributor

@dylanhmorris dylanhmorris left a comment

Choose a reason for hiding this comment

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

Thanks, @jkislin! Should be good to go with deletion of old shell script

Copy link
Contributor

@dylanhmorris dylanhmorris left a comment

Choose a reason for hiding this comment

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

LGTM!

@dylanhmorris dylanhmorris merged commit 66c0c1e into main Jan 5, 2026
125 of 126 checks passed
@dylanhmorris dylanhmorris deleted the jk-rm-cache branch January 5, 2026 18:04
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.

4 participants