-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
TODO: Refactor container config #7027
TODO: Refactor container config #7027
Conversation
/assign @baude looks like test failed, but a gating test on a file not affiliated with this change. |
@mheon PTAL |
Ah, I think you broke some test functions that want to make Container structs - looks like |
Will take a look at fixing 👍 |
cfae4c2
to
3abdaf9
Compare
friendly ping, all tests fixed. |
@ldelossa needs a rebase. |
This commit handle the TODO task of breaking the Container config into smaller sub-configs Signed-off-by: ldelossa <[email protected]>
3abdaf9
to
10c4ab1
Compare
Done. Updated a few |
We really need to do a pass over comments on individual fields in the structs, but that's a lot of work and can happen later. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ldelossa, mheon 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 |
@mheon As I am just getting acquainted with this code base, if you have an idea of a good starting point for a maintenance task like that, creating an issue and assigning it to me would be welcome on my end. thanks for approval. |
looks like all tests passed aside from one, maybe a flake? |
Out of a |
Aaaaargh! Yes, it is. A bug in crun which I had hoped would be fixed in CI after #6822 got merged yesterday. Looking at the failing results, I see |
Cirrus CI / test fedora-31 TEST_REMOTE_CLIENT:false started 23m 8s ago So ~23m ago it failed. |
No, that's just the time when the test was restarted. It's still running. I'll check it when it finishes. |
got it, my bad. |
Test passed, but package versions shows crun 0.13. Looks like crun 0.14 made it only into f32, not f31. I'll pursue that. |
This good to get merged? |
Sure, LGTM. |
Thanks @ldelossa LGTM |
This commit handle the TODO task of breaking the Container
config into smaller sub-configs.
Signed-off-by: ldelossa [email protected]