remove deadcode #26461
Open
remove deadcode #26461
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The functionwas added but never wired into the cli option so there never where shell completions for this. Signed-off-by: Paul Holzinger <[email protected]>
These functions are not used. Signed-off-by: Paul Holzinger <[email protected]>
Deadcode should that the ShouldRestart() API endpoint was never wired into the router so the endpoint did not existed and the bindings called a non existing endpoint which returnd 404 which the binding code assumed means no restart. As such remove all this code as it didn't do anything useful. And IMO exposing a shouldrestart API always feeled wrong to me. The client should not have to deal with this. This commit does not change the behavior but it also does not make an attempt to fix the broken restart handling with the rmeote client. Given we do not seem to have any user reports about this it seems it is not used. Signed-off-by: Paul Holzinger <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99 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
Luap99
added
the
No New Tests
github-actions
bot
added
machine
kind/api-change
One might think Close() should be called but we are already using Shutdown() which is the graceful way to stop the server so we don't actually need Close(). Signed-off-by: Paul Holzinger <[email protected]>
Only one function, there are more public bindings that call a legit server endpoint but are unused by podman-remote. As external users might need/want them they should stay. Signed-off-by: Paul Holzinger <[email protected]>
Signed-off-by: Paul Holzinger <[email protected]>
Make use of our custom ChoiceValue flag type instead of using yet another type. With that we can remove the StringSet type. Signed-off-by: Paul Holzinger <[email protected]>
Signed-off-by: Paul Holzinger <[email protected]>
Note sure what these are supposed to be used for but they are unused. Signed-off-by: Paul Holzinger <[email protected]>
We don't need a stub implementation as this code should never end up being imported on non windows platforms. Signed-off-by: Paul Holzinger <[email protected]>
Can always added back if it is really needed at some point. Signed-off-by: Paul Holzinger <[email protected]>
Yes this is a lot. Signed-off-by: Paul Holzinger <[email protected]>
These types are not used. Signed-off-by: Paul Holzinger <[email protected]>
Signed-off-by: Paul Holzinger <[email protected]>
Signed-off-by: Paul Holzinger <[email protected]>
It is not used here. Signed-off-by: Paul Holzinger <[email protected]>
Signed-off-by: Paul Holzinger <[email protected]>
Signed-off-by: Paul Holzinger <[email protected]>
Signed-off-by: Paul Holzinger <[email protected]>
All callers ignore the error anyways so no reason to return it as the function itself already logs it. Signed-off-by: Paul Holzinger <[email protected]>
mheon
reviewed
Jun 18, 2025
| @@ -382,20 +352,6 @@ func WithTmpDir(dir string) RuntimeOption { | |||
| } | |||
| } | |||
|
|
|||
| // WithNoPivotRoot sets the runtime to use MS_MOVE instead of PIVOT_ROOT when | |||
| // starting containers. | |||
| func WithNoPivotRoot() RuntimeOption { | |||
There was a problem hiding this comment.
Wait what? This is kind of important. How is it not wired in?
There was a problem hiding this comment.
there is no cli option for this is there? The code reads config.Engine.NoPivotRoot directly from containers.conf so this is not adding anything AFAICT
|
While I love the diffstat, I'm a bit concerned that some of these things aren't wired in... |
|
Speaks to our code quality/review process I guess... I am fine dropping individual changes here if you want to keep something. And frankly I haven't even dropped everything, I completely ignored pkg/k8s.io for example. Mostly removed stuff I don't see any use for. |
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
|
lgtm |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
see each commit, mostly straight forward removals however "podman images --sort autocomplete options", "remove unused ShouldRestart() code" and "podman images --sort use ChoiceValue flag" contain some small changes that I noticed during the removals.
Does this PR introduce a user-facing change?