-
Notifications
You must be signed in to change notification settings - Fork 196
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
chore(deps): update dependency containers/automation_images to v20240529 #2024
Conversation
I think the failure is (log):
Not something I can fix. |
This means that the VMs are using the containers.conf from Rawhide? |
We probably should change the default in containers.conf to zstd:chunked and then change containers.conf for RHEL and Fedora < 41 to default to gzip. |
This count as breaking change to me. The upstream defaults will be consumed by all distros so if we change this in a minor release we could make many people unhappy if it breaks for whatever reasons. Having a documented process to turn it on for the next fedora release (https://fedoraproject.org/wiki/Changes/zstd:chunked) is fine but forcing this into all distros maybe not so much. Looking at containers/storage#1928 there seem to be cases where it performs much worse currently. |
Regardless of this specific case unit tests should never depend on installed system configs at all. So the proper fix is to make the unit tests not read system configs and use them to check defaults as this is broken no matter what and true for every config field we have |
I agree, there is no point in what we are testing now. Should we just use diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go
index bacab043..f48c085e 100644
--- a/pkg/config/config_test.go
+++ b/pkg/config/config_test.go
@@ -21,7 +21,7 @@ var _ = Describe("Config", func() {
It("should succeed with default config", func() {
// Given
// When
- defaultConfig, err := NewConfig("")
+ defaultConfig, err := NewConfig("containers.conf") |
NewConfig() still reads all system configs by default + the given path so it doesn't help us here unfortunately. |
If we tested "/dev/null" we would get the default config |
652897e
to
5c512cc
Compare
b80ebd0
to
d378558
Compare
d378558
to
ce76874
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...deleted...
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: cevich, renovate[bot] The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ISTM like something might just need updating in the tests:
|
6e3ee89
to
be07cb8
Compare
@vrothberg @Luap99 @rhatdan any hope of fixing the problem here? c/common is a pretty low-level dep. It would be nice to have it using the latest CI VM images everything else is already using. |
see comments above for how to fix. In the short time we get a working f40 containers-common again, ref #2048, |
Oh, that I know how to do.
I'm no expert, but skimming the comments above, it sounds like that might also be desirable? |
be07cb8
to
de97d83
Compare
Force-push: Jammed in a recent image set from containers/automation_images#354 (comment) |
de97d83
to
4a61f58
Compare
Ugh, since renovate just overwrote my commit w/ newer images, they do indeed pass the F40 test. |
...deleted: wrong tab... |
9bd0ad5
to
de69246
Compare
Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
de69246
to
6ed7775
Compare
#2059 will fix this. |
This PR contains the following updates:
20240513t140131z-f40f39d13
->20240529t141726z-f40f39d13
Release Notes
containers/automation_images (containers/automation_images)
v20240529t141726z-f40f39d13
Compare Source
Configuration
📅 Schedule: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).
🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.
♻ Rebasing: Whenever PR is behind base branch, or you tick the rebase/retry checkbox.
🔕 Ignore: Close this PR and you won't be reminded about this update again.
This PR has been generated by Mend Renovate. View repository job log here.