-
Notifications
You must be signed in to change notification settings - Fork 198
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
Remove ActiveDestination method to move into podman #1640
Conversation
LGTM |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgiradkar, rhatdan 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 |
d55bdca
to
8b41842
Compare
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.
Going one step back:
Should we consider to ignore the existence of these environment variables here entirely? I feel using them here AND over in Podman is just making the logic more complicated than it needs to.
The function here returns whatever Podman asks for. What Podman asks for is up to Podman code to decide. I may be missing something important. @ashley-cui @rhatdan @cgiradkar what do you think?
Computationally it should not make any difference but it may lead to better code readability. Additionally if the method ActiveDestination is only getting used in Podman repo, then, I think, we can move the logic to Podman. |
44fb7a0
to
c98bdcf
Compare
c98bdcf
to
88f712b
Compare
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
/lgtm |
88f712b
to
f788fe5
Compare
@rhatdan can you please re-approve? I pushed a small linter change. |
Please clean this up, we want to get this merged before we cut the release. |
I see no reason to declare this as blocker. |
No it is not a blocker, but does fix a Bug that would be nice to get fixed before the release. When I mark something as 4.7 it is not a blocker, but something that we should consider for 4.7. |
f788fe5
to
2e6937d
Compare
The method ActiveDestination was being used only by Podman and there seemed to code complications as the code is split in multiple parts. Hence, moved the code to Podman to make it more readable and efficient. Signed-off-by: Chetan Giradkar <cgiradka@redhat.com>
2e6937d
to
d64e04a
Compare
The method ActiveDestination was being used only by Podman and there seemed to code complications as the code is split in multiple parts. Hence, moved the code to Podman to make it more readable and efficient. |
/lgtm |
/hold cancel |
This commit won't compile in Podman anymore. Please open a PR against Podman ASAP to clean things up. |
Opened a PR #20084 in podman, waiting for CI to do its magic. |
The method ActiveDestination was being used only by Podman and there seemed to code complications as the code is split in multiple parts. Hence, moved the code to Podman to make it more readable and efficient.
relates to: #15588
Signed-off-by: Chetan Giradkar cgiradka@redhat.com