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.

Sign up for GitHub

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

Jump to bottom

vendor: bump c/buildah to v1.30.1-0.20230522160735-eda32d4a2c72 #18579

Closed
wants to merge 2 commits into from
Closed

vendor: bump c/buildah to v1.30.1-0.20230522160735-eda32d4a2c72 #18579

wants to merge 2 commits into from

Conversation

flouthoc
Copy link
Collaborator

@flouthoc flouthoc commented May 16, 2023 •
edited
Loading

Does this PR introduce a user-facing change?

vendor: bump c/buildah to v1.30.1-0.20230522160735-eda32d4a2c72

Add changes after: containers/buildah#4792

@openshift-ci openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 16, 2023
@flouthoc flouthoc mentioned this pull request May 16, 2023
Merged
@flouthoc flouthoc force-pushed the validate-volume-backend branch from 30d0e39 to aed2f24 Compare May 17, 2023 08:51
@flouthoc flouthoc changed the title [DO NOT MERGE] vendor: bump c/buildah to flouthoc/buildah vendor: bump c/buildah to v1.30.1-0.20230516202929-8ca9e72f2c47 May 17, 2023
@flouthoc
Copy link
Collaborator Author

@containers/podman-maintainers PTAL

@flouthoc flouthoc force-pushed the validate-volume-backend branch from aed2f24 to e7b8f7d Compare May 17, 2023 08:53
vrothberg
vrothberg approved these changes May 17, 2023
View reviewed changes
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
/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 May 17, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 17, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 17, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, vrothberg

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

@Luap99
Copy link
Member

Luap99 commented May 17, 2023

Can you add a test for it? If you require two systems (with different directories) you could use the machine tests for it.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 17, 2023
@flouthoc flouthoc force-pushed the validate-volume-backend branch 3 times, most recently from 05441fc to 60ed2d5 Compare May 18, 2023 09:09
@flouthoc
Copy link
Collaborator Author

Okay @Luap99 @containers/podman-maintainers PTAL again, last two failures are flakes.

@flouthoc flouthoc mentioned this pull request May 18, 2023
Draft
@flouthoc
Copy link
Collaborator Author

No clue why is it fetching debian:testing-slm from docker and then trying to read from quay.io

Error: creating build container: initializing source docker://debian:testing-slim: reading manifest testing-slim in quay.io/libpod/debian: manifest unknown

@flouthoc
Copy link
Collaborator Author

@vrothberg @mtrmac Any clue how could this be happening ? fetching image from a different repo and performing lookup in quay.io ?

@mtrmac
Copy link
Collaborator

mtrmac commented May 19, 2023

That means there is a redirect or a mirror set up in registries.conf. From a quick search,

podman/test/registries.conf

Lines 15 to 17 in a58ea23

[[registry]]
prefix="docker.io/library"
location="quay.io/libpod"
seems related.

Luap99
Luap99 reviewed May 19, 2023
View reviewed changes
test/e2e/build_test.go Outdated
@@ -267,6 +267,23 @@ var _ = Describe("Podman build", func() {
Expect(session.OutputToString()).To(ContainSubstring("hello"))
})

It("podman build test volume mount on server end", func() {
Copy link
Member

Choose a reason for hiding this comment

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

This passes on main which means you are not testing the bug!

The issue #17139 shows that you need to have client and server on different machines. You can never test it like this. You need to have a client where the given -v path does not exist and it only exist on the serve side.
You can add a test like this in pkg/machine/e2e as this runs on actually runs two different systems.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes adding a mount on machine side and then using it in build SGTM, let me amend this.

@flouthoc flouthoc mentioned this pull request May 19, 2023
Merged
@TomSweeneyRedHat
Copy link
Member

TomSweeneyRedHat commented May 19, 2023 •
edited
Loading

LGTM
after the test is amended

@flouthoc flouthoc force-pushed the validate-volume-backend branch 5 times, most recently from 5c999eb to 505866d Compare May 22, 2023 14:59
@rhatdan
Copy link
Member

rhatdan commented May 22, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 22, 2023
@flouthoc flouthoc force-pushed the validate-volume-backend branch from 505866d to 76d2a6c Compare May 22, 2023 16:39
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 22, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 22, 2023

New changes are detected. LGTM label has been removed.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 22, 2023
flouthoc added 2 commits May 22, 2023 22:11
@flouthoc
vendor: bump c/buildah to v1.30.1-0.20230522160735-eda32d4a2c72
ffdc5de 
Signed-off-by: Aditya R <[email protected]>
@flouthoc
test: volume mount on remote end
ae748a2 
Volume mount on remote must be validated on backend.

Signed-off-by: Aditya R <[email protected]>
@flouthoc flouthoc force-pushed the validate-volume-backend branch from 76d2a6c to ae748a2 Compare May 22, 2023 16:41
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 22, 2023
@flouthoc flouthoc changed the title vendor: bump c/buildah to v1.30.1-0.20230516202929-8ca9e72f2c47 vendor: bump c/buildah to v1.30.1-0.20230522160735-eda32d4a2c72 May 22, 2023
@edsantiago edsantiago added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/blocked-paths Indicates that a PR should not merge because it touches files in blocked paths. do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. do-not-merge/needs-unit-tests do-not-merge/needs-integration-tests do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None do-not-merge/needs-confirmation Waiting for feedback from stakeholder before merging labels May 22, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label May 22, 2023
@edsantiago
Copy link
Member

This must not merge. See #18631.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2023
@openshift-merge-robot
Copy link
Collaborator

PR needs rebase.

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.

@rhatdan
Copy link
Member

rhatdan commented Jun 13, 2023

I don't believe this is still needed, since I am working on a separate PR.

@rhatdan rhatdan closed this Jun 13, 2023
@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 12, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Luap99 Luap99 Luap99 left review comments

@vrothberg vrothberg vrothberg approved these changes

Assignees

@rhatdan rhatdan

@vrothberg vrothberg

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/blocked-paths Indicates that a PR should not merge because it touches files in blocked paths. do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. do-not-merge/needs-confirmation Waiting for feedback from stakeholder before merging do-not-merge/needs-integration-tests do-not-merge/needs-unit-tests do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@flouthoc @Luap99 @mtrmac @TomSweeneyRedHat @rhatdan @edsantiago @openshift-merge-robot @vrothberg