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

Isolation strings, should match user input #3033

Merged
merged 1 commit into from Feb 23, 2021

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Feb 23, 2021

When we parse isolation we expect users to input chroot, oci, rootless.

So when we translate the constants back to strings, we should use the
same values.

These human names need to be passed over the podman-remote build
bindings, so we need to make them match.

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake
/kind other

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?


Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -959,7 +959,7 @@ func defaultIsolation() (define.Isolation, error) {
func IsolationOption(isolation string) (define.Isolation, error) {
if isolation != "" {
switch strings.ToLower(isolation) {
case "oci":
case "oci", "default":
Copy link
Member

Choose a reason for hiding this comment

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

based on your changes above, is "default" possible?

Copy link
Member

Choose a reason for hiding this comment

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

I wonder the same, but otherwise LGTM.

@TomSweeneyRedHat
Copy link
Member

Lint isn't happy, perhaps due to the "default" check? The error is not very enlightening.

@rhatdan rhatdan force-pushed the isolation branch 2 times, most recently from 1bcf06d to d138064 Compare February 23, 2021 16:43
When we parse isolation we expect users to input chroot, oci, rootless.

So when we translate the constants back to strings, we should use the
same values.

These human names need to be passed over the podman-remote build
bindings, so we need to make them match.

Also docker describes an isolation of "default", which we should also
handle for potential scripts.

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
@rhatdan
Copy link
Member Author

rhatdan commented Feb 23, 2021

@containers/podman-maintainers PTAL

@rhatdan
Copy link
Member Author

rhatdan commented Feb 23, 2021

@jwhonce
Copy link
Member

jwhonce commented Feb 23, 2021

/lgtm

@openshift-ci-robot
Copy link
Collaborator

@jwhonce: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link
Member

@lsm5 lsm5 left a comment

Choose a reason for hiding this comment

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

LGTM. /cc @ashcrow

@lsm5
Copy link
Member

lsm5 commented Feb 23, 2021

/lgtm
/approve

@lsm5
Copy link
Member

lsm5 commented Feb 23, 2021

Needs approval from an approver in each of these files:

* **[OWNERS](https://github.com/containers/buildah/blob/master/OWNERS)**

my vote doesn't count :| .. oh well..

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: lsm5, 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-merge-robot openshift-merge-robot merged commit d5c503c into containers:master Feb 23, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants