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

builder: create WORKDIR with USER ownership #219

Merged
merged 3 commits into from
Mar 29, 2022

Conversation

giuseppe
Copy link
Member

let the executor deal with creating the WorkingDirectory.

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

@giuseppe
Copy link
Member Author

I've tried the reproducer here #87 and could not reproduce the issue.

Needed for: containers/buildah#3840

@rhatdan
Copy link
Contributor

rhatdan commented Mar 23, 2022

LGTM, but we need @nalind opinion on this.

@nalind
Copy link
Member

nalind commented Mar 23, 2022

Is there an explanation of why this change is being made? Conformance tests don't get run in CI; do they continue to pass with this change?

giuseppe added a commit to giuseppe/buildah that referenced this pull request Mar 24, 2022
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe
Copy link
Member Author

Is there an explanation of why this change is being made? Conformance tests don't get run in CI; do they continue to pass with this change?

if we postpone the mkdir to containers/buildah#3840 then we can create it with the correct ownership.

Do I need Docker to run the conformance tests? I've pointed it to Podman (running locally with sudo podman system service -t 0 unix:///var/run/docker.sock) and I get all sort of failures.

I've vendored this change into containers/buildah#3840 to run the conformance tests there

@giuseppe
Copy link
Member Author

Do I need Docker to run the conformance tests? I've pointed it to Podman (running locally with sudo podman system service -t 0 unix:///var/run/docker.sock) and I get all sort of failures.

stupid question. We clearly need Docker, going to test with that.

giuseppe added a commit to giuseppe/buildah that referenced this pull request Mar 24, 2022
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe
Copy link
Member Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 24, 2022
giuseppe added a commit to giuseppe/buildah that referenced this pull request Mar 24, 2022
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
giuseppe added a commit to giuseppe/buildah that referenced this pull request Mar 24, 2022
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@nalind
Copy link
Member

nalind commented Mar 24, 2022

if we postpone the mkdir to containers/buildah#3840 then we can create it with the correct ownership.

Please make sure that's noted in the commit log.

giuseppe added a commit to giuseppe/buildah that referenced this pull request Mar 24, 2022
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
giuseppe added a commit to giuseppe/buildah that referenced this pull request Mar 24, 2022
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe giuseppe force-pushed the drop-ensure-container-path branch 3 times, most recently from 890a9bb to 3a821e1 Compare March 24, 2022 15:46
giuseppe added a commit to giuseppe/buildah that referenced this pull request Mar 24, 2022
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
builder.go Show resolved Hide resolved
@rhatdan
Copy link
Contributor

rhatdan commented Mar 25, 2022

@giuseppe could you force push this to trigger CI.

@rhatdan
Copy link
Contributor

rhatdan commented Mar 25, 2022

/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 25, 2022
giuseppe added a commit to giuseppe/buildah that referenced this pull request Mar 25, 2022
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
giuseppe added a commit to giuseppe/buildah that referenced this pull request Mar 25, 2022
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
giuseppe added a commit to giuseppe/buildah that referenced this pull request Mar 28, 2022
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Copy link
Contributor

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

LGTM @nalind PTAL

@giuseppe giuseppe changed the title builder: drop EnsureContainerPath builder: create WORKDIR with USER ownership Mar 28, 2022
dockerclient/client.go Outdated Show resolved Hide resolved
giuseppe added a commit to giuseppe/buildah that referenced this pull request Mar 28, 2022
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
dockerclient/client.go Outdated Show resolved Hide resolved
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 28, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, giuseppe, 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
Copy link
Contributor

openshift-ci bot commented Mar 28, 2022

@giuseppe: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@TomSweeneyRedHat
Copy link
Contributor

LGTM

@giuseppe
Copy link
Member Author

fixed, Buildah tests are also green: containers/buildah#3840

@rhatdan rhatdan merged commit 9afddc0 into openshift:master Mar 29, 2022
@nalind
Copy link
Member

nalind commented Mar 29, 2022

Did we run the conformance tests?

@giuseppe
Copy link
Member Author

I've used this branch in containers/buildah#3840

@nalind
Copy link
Member

nalind commented Mar 29, 2022

I was referring to this repository's conformance tests, which CI doesn't run.

@nalind
Copy link
Member

nalind commented Mar 29, 2022

Something like nalind@b2bca3b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants