-
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
Option --url and --connection should imply --remote. #13296
Option --url and --connection should imply --remote. #13296
Conversation
d7ffc25
to
049b6df
Compare
Closes containers#13242 Signed-off-by: Romain Geissler <[email protected]>
049b6df
to
23a7f4e
Compare
@jwhonce PTAL |
LGTM |
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.
Can you add a comment in root.go
Line 342 in a88ea2c
connectionFlagName := "connection" |
otherwise LGTM
To be honest if this was one of my C++ projects, I would ask my guys to somehow define this const char* as a global variable somewhere and reuse it in both files. I know nothing at all about go, but most likely I guess there is a way to define a global const variable and if yes I will most likely find it by Googling a bit. Do you want to me introduce a const global variable instead ? |
Sure, make it a global const but this does not fix the problem that somebody might change the flag in one place but not the other. |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan, Romain-Geissler-1A 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 |
Closes #13242
Note: I am not sure how to actually test anything, I blindly tried to add a test, hoping your CI will run it for me !