-
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
[v4.6] fix(env): parsing --env incorrect in cli #19566
[v4.6] fix(env): parsing --env incorrect in cli #19566
Conversation
Close: containers#18724 Signed-off-by: Black-Hole1 <[email protected]>
First, all the defaults for TERM=xterm were removed from c/common, then accordingly the same will be added if encountered a set tty flag. Signed-off-by: Chetan Giradkar <[email protected]>
Backports a last minute issue found and corrected this morning on environment variable handling. It didn't go cleanly, so I've also backported two other recent commits to make it clean that also are related. The commits in order of submission are: feat(env): support multiline in env-filefeat(env): support multiline in env-file - containers@170a786 Check tty flag to set default terminal in Env - containers@53d44a6 fix(env): parsing --env incorrect in cli - containers@7ce654f Signed-off-by: Black-Hole1 <[email protected]> Signed-off-by: TomSweeneyRedHat <[email protected]>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: TomSweeneyRedHat 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 |
@vrothberg @BlackHole1 PTAL |
@edsantiago PTAL Ed has found some more issues, so I prefer him to weigh in. |
Please do not merge this. #19096 never made it into 4.6, so this "fix" is not necessary. And, env-file is pretty badly broken IMO (#19565), we need to get it properly sorted out. I may be mistaken, so I'm leaving this open but with lots of DO-NOT-MERGE tags. If someone believes this code is absolutely needed in 4.6, please correct me. |
I agree with @edsantiago's perspective. Currently, there are some disagreements regarding the implementation of env-file. We should wait until we reach a consensus before making a decision. |
This should not get backported. It has a high risk of breakage which makes it not suitable for a patch release IMO. |
I agree if there was no change in this functionality in 4.6 then it should NOT be back ported. |
Sold, I was backporting just based on Matt's note, and then what I thought was needed to keep the commits happy at the end. We'll leave this to 4.7 then. Thanks for the discussion. |
Backports a last-minute issue found and corrected this morning
on environment variable handling. It didn't go cleanly, so I've
also backported two other recent commits to make it clean that
also are related. The commits in order of submission are:
feat(env): support multiline in env-filefeat(env): support multiline in env-file - 170a786
Check tty flag to set default terminal in Env - 53d44a6
fix(env): parsing --env incorrect in cli - 7ce654f
Signed-off-by: Black-Hole1 [email protected]
Signed-off-by: TomSweeneyRedHat [email protected]
Does this PR introduce a user-facing change?