Skip to content
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 dead RuntimeOption functions #12772

Merged
merged 1 commit into from
Jan 10, 2022

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Jan 7, 2022

I don't see where these With Functions are used, so removing them to
clean up code.
WithDefaultInfra* functions screwed me up and confused me.

[NO NEW TESTS NEEDED]

Signed-off-by: Daniel J Walsh [email protected]

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 7, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 7, 2022
@rhatdan
Copy link
Member Author

rhatdan commented Jan 7, 2022

@mheon PTAL

@mheon
Copy link
Member

mheon commented Jan 7, 2022

A lot of these are unused but I added them in case someone wanted to use Libpod directly, setting configuration options they required. I don't know if we want to remove those.

There are definitely some that are just unused now, though.

@TomSweeneyRedHat
Copy link
Member

I think I'd leave them unless some of them we can ever forsee using. I've not used libpod directly much myself, but could see a case where some of these would be useful.

@rhatdan
Copy link
Member Author

rhatdan commented Jan 8, 2022

Well the Infra ones should definely go since these are part of the Pod not part of the runtime.

@@ -115,19 +115,6 @@ func WithStorageConfig(config storage.StoreOptions) RuntimeOption {
}
}

// WithDefaultTransport sets the default transport for retrieving images.
func WithDefaultTransport(defaultTransport string) RuntimeOption {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is actually honored anymore (@vrothberg can confirm). We might want to fix it, though, it does live in containers.conf and setting it there has no effect.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not honored anymore, correct. I am not sure if we should continue supporting it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't want it it should be pulled from c/common as well. Still got a few more weeks of 4.0 prep to make breaking changes.

// they will not share containers, and unspecified behavior may occur.
func WithStateType(storeType config.RuntimeStateStore) RuntimeOption {
return func(rt *Runtime) error {
if rt.valid {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can get rid of this one.

@@ -199,20 +166,6 @@ func WithConmonPath(path string) RuntimeOption {
}
}

// WithConmonEnv specifies the environment variable list for the conmon process.
func WithConmonEnv(environment []string) RuntimeOption {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is potentially useful, keep it.

@@ -260,21 +213,6 @@ func WithCgroupManager(manager string) RuntimeOption {
}
}

// WithStaticDir sets the directory that static runtime files which persist
// across reboots will be stored.
func WithStaticDir(dir string) RuntimeOption {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep this one

// WithNoPivotRoot sets the runtime to use MS_MOVE instead of PIVOT_ROOT when
// starting containers.
func WithNoPivotRoot() RuntimeOption {
return func(rt *Runtime) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep this one

// WithCNIPluginDir sets the CNI plugins directory.
func WithCNIPluginDir(dir string) RuntimeOption {
return func(rt *Runtime) error {
if rt.valid {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep this one

if rt.valid {
return define.ErrRuntimeFinalized
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep this one

// An infra image is used for inter-container kernel
// namespace sharing within a pod. Typically, an infra
// container is lightweight and is there to reap
// zombie processes within its pid namespace.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be gotten rid of


// WithDefaultInfraCommand sets the command to
// run on pause container start up.
func WithDefaultInfraCommand(cmd string) RuntimeOption {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one still makes sense, I think.

}

// WithDefaultInfraName sets the infra container name for a single pod.
func WithDefaultInfraName(name string) RuntimeOption {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one can be removed

I don't see where these With Functions are used, so removing them to
clean up code.
WithDefaultInfra* functions screwed me up and confused me.

[NO NEW TESTS NEEDED]

Signed-off-by: Daniel J Walsh <[email protected]>
@mheon
Copy link
Member

mheon commented Jan 10, 2022

LGTM

@jwhonce
Copy link
Member

jwhonce commented Jan 10, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2022
@openshift-merge-robot openshift-merge-robot merged commit 87cd4b6 into containers:main Jan 10, 2022
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants