-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Proof of concept: nightly dependency treadmill #15910
Proof of concept: nightly dependency treadmill #15910
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
c4d1b2b
to
da331be
Compare
This is kind of weird: it compiles, and almost all the Anyhow, gotta go. Thoughts welcome on this approach. |
I would figure the validate tests would fail on this with unvendored content? |
That's what the |
We need to commit a breaking change to make sure this fails. :^) |
da331be
to
cf8ffd8
Compare
Re-pushed on 40e8bcb, passed again. (With two flakes, both of them nasty ones, but I'll follow up on Monday). |
cf8ffd8
to
28b45eb
Compare
@containers/podman-maintainers hi! I re-pushed during scrum, and, lo, it's failing:
also
Vendor diffs here. I restarted each one, failed the same way. Does not seem to be a flake. |
Another failure, in
|
culprit (for at least one of the failures, haven't checked the others) seems to be containers/storage#1357 |
Do the tests have XDG_CONFIG_HOME set and no storage.conf defined? |
I'm guessing you don't need an answer, because you submitted containers/storage#1363 11 minutes after this question, but: using |
Weird that XDG_CONFIG_HOME is now showing it is set. Will see if updated PR to storage fixes the problem. |
And it's not even nightly yet! P.S. does this help? Lines 123 to 163 in aaeabb0
|
So it is self inflicted. |
ISTR a very very good reason for adding that. tig blame dates it to #3466, August 2019. |
|
33a2137
to
9ece902
Compare
This is ready for review, and to merge as soon as CI passes (which it will, because the |
# Big Three dependencies, and run full CI test suite. Notification | ||
# email will go out to monitor-list upon failure. | ||
if [[ "$CIRRUS_CRON" = "treadmill" ]]; then | ||
for pkg in common image/v5 storage; do |
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.
Why is buildah excluded? This will fail if common makes a breaking change that also affects buildah.
IMO we need to vendor everything at once, excluding buildah will cause even more troubles.
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.
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.
Not really, this thing here will just start to fail we break something in c/common. Until common is declared stable this job will fail regularly.
Is there any reason to not just vendor everything in your treadmill PR vs this automated job?
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.
Is there any reason to not just vendor everything in your treadmill PR vs this automated job?
My sanity. Buildah treadmill is already a pretty big time suck (some days). I'm reluctant to spend more time on this sort of integration work.
LGTM |
There's one problem with this, that I haven't been able to figure out: immediately after a vendor-everything-into-podman PR, this nightly cron job is going to fail, because I (deliberately) did not add
I was trying to save the planet. Now I'm thinking it's more important to save human time. Thoughts? |
We're doomed, one way or another. So I think you should have a good time and not be bothered by false positives. |
Agreed. Humans should be primary. |
9ece902
to
c370fce
Compare
So done. Rebased & repushed with |
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.
/lgtm
As discussed in f2f: this is the cleanest, simplest mechanism I can think of to auto-test the Big Three dependencies: simply run go mod edit immediately after git checkout, then run the entire CI test suite. This differs significantly from the buildah treadmill, in that buildah is almost impossible to re-vendor without manual intervention. (In practice, so are these, but let's dream of a world in which this will run and pass every night). (I want a pony too). Signed-off-by: Ed Santiago <[email protected]>
c370fce
to
1e71d12
Compare
This has been working well in buildah. Apart from, sigh, flakes. |
/lgtm |
As discussed in f2f: this is the cleanest, simplest mechanism
I can think of to auto-test the Big Three dependencies: simply
run go mod edit immediately after git checkout, then run the
entire CI test suite.
If this approach works, we can set up a new CIRRUS_CRON=treadmill
job. I'm expecting it not to work, because Murphy, but want to
see what the unexpected gotchas are.
This differs significantly from the buildah treadmill, in that
buildah is almost impossible to re-vendor without manual intervention.
(In practice, so are these, but let's dream of a world in which
this will run and pass every night). (I want a pony too).
Signed-off-by: Ed Santiago [email protected]