-
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
Quadlet - prefer "param val" over "param=val" to allow env expansion #24128
Quadlet - prefer "param val" over "param=val" to allow env expansion #24128
Conversation
When possible use a generic function to add strings and booleans Adjust tests Signed-off-by: Ygal Blum <[email protected]>
Ephemeral COPR build failed. @containers/packit-build please check. |
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, I really like the map approach.
I think for future work we should consider splitting the maps with common options between pod/container out so they both can use the same thing and we do not need to add such option in both places all the time, especially as pod untis currently only supported limed keys. Of course there will still be a bunch of work in adding tests/docs then but hopefully that can be improved as well at some point
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, ygalblum 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 |
LGTM |
@rhatdan The tests are fine, it's the packit service that's been acting out for a while now. I can see that it's getting 503 when trying to create the build root |
/lgtm |
4eb43de
into
containers:main
@Luap99 you are right. There is more common code in the |
When possible use a generic function to add strings and booleans
Adjust tests
Does this PR introduce a user-facing change?
No
Resolves: #24105