kube play: complete container spec #17107
Conversation
Make sure that the specs of containers generated by `kube play` are correctly completed. They have not before which surfaced in default environment variables not being set. Fixes: containers#17016 Signed-off-by: Valentin Rothberg <[email protected]>
|
@Luap99 @rhatdan @umohnani8 PTAL |
openshift-ci
bot
added
release-note
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vrothberg 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 |
openshift-ci
bot
added
the
approved
| for _, w := range warn { | ||
| fmt.Fprintf(os.Stderr, "%s\n", w) | ||
| } |
There was a problem hiding this comment.
I see the other callers do the same but shouldn't we use logrus.Warn() for this? It feels very ugly to just print somewhere in the backend where the callers have no control over it. I would argue that podman --log-level=error should not show such messages if they are just warnings.
There was a problem hiding this comment.
I also don't like printing to stderr. But as you mentioned, it's consistent with other call sites, so I didn't change it.
I am supportive of changing those to logrus.Warn*. There's probably more such cases across the code base.
|
LGTM once the comment on logging is addressed (personally, don't have a preference, good to merge with either) |
|
Let’s do that in another PR
…On Fri 13 Jan 2023 at 20:16, Matt Heon ***@***.***> wrote:
LGTM once the comment on logging is addressed (personally, don't have a
preference, good to merge with either)
—
Reply to this email directly, view it on GitHub
<#17107 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACZDRA45G3XIIROTCECBAO3WSGSXNANCNFSM6AAAAAAT2OW2BU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
LGTM |
|
/lgtm |
|
/hold cancel |
rhatdan
removed
the
do-not-merge/work-in-progress
github-actions
bot
added
the
locked - please file new issue/PR
Make sure that the specs of containers generated by
kube playare correctly completed. They have not before which surfaced in default environment variables not being set.Fixes: #17016
Signed-off-by: Valentin Rothberg [email protected]
Does this PR introduce a user-facing change?