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

Add configuration for podmansh #22683

Merged
merged 3 commits into from
May 23, 2024
Merged

Conversation

grisu48
Copy link
Contributor

@grisu48 grisu48 commented May 13, 2024

Use the configuration parameters from the newly introduced
PodmanshConfig struct. This allows podmansh to be configured via
configuration files.

Configuring the shell is a feature I require for my persistent user container project I'm currently working on, see also
https://lists.podman.io/archives/list/podman@lists.podman.io/thread/VCVWFK37UGW434KNE46MDK6NOE6CPHRB/

Merge together with containers/common#1993 (merged)

Does this PR introduce a user-facing change?

Adds configuration options for the container name and shell in podmansh

Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

cmd/podman/main.go Outdated Show resolved Hide resolved
@grisu48 grisu48 marked this pull request as draft May 15, 2024 14:12
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 15, 2024
@grisu48 grisu48 marked this pull request as ready for review May 16, 2024 13:12
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 16, 2024
@mheon
Copy link
Member

mheon commented May 16, 2024

You have build failures. Possibly needs a new containers/common vendor?

@grisu48
Copy link
Contributor Author

grisu48 commented May 21, 2024

You have build failures. Possibly needs a new containers/common vendor?

Yes, this is together with containers/common#1993.

@grisu48 grisu48 marked this pull request as draft May 21, 2024 08:13
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 21, 2024
@grisu48 grisu48 marked this pull request as ready for review May 21, 2024 09:13
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 21, 2024
@grisu48 grisu48 marked this pull request as draft May 21, 2024 12:03
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 21, 2024
@grisu48 grisu48 force-pushed the podmansh_sh branch 2 times, most recently from 7c7916f to 9039d69 Compare May 21, 2024 12:25
@grisu48 grisu48 marked this pull request as ready for review May 23, 2024 06:04
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 23, 2024
@grisu48
Copy link
Contributor Author

grisu48 commented May 23, 2024

containers/common#1993 is merged. Waiting for renovate to update the containers/common dependency then this can me merged as well.

@Luap99
Copy link
Member

Luap99 commented May 23, 2024

containers/common#1993 is merged. Waiting for renovate to update the containers/common dependency then this can me merged as well.

You can add second commit here that vendors c/common@main (needs to ensure the commit is before this one to ensure it builds correctly)

@grisu48
Copy link
Contributor Author

grisu48 commented May 23, 2024

containers/common#1993 is merged. Waiting for renovate to update the containers/common dependency then this can me merged as well.

You can add second commit here that vendors c/common@main (needs to ensure the commit is before this one to ensure it builds correctly)

Done. I've run a go get github.com/containers/common@79d954c7766303997d7dea4340442855e143fc5b with the last commit from https://github.com/containers/common/ and created a separate commit for the changes therein. Two additional repositories got updated, I assume because they are dependencies.

Please help me verify if this is correct.

@Luap99
Copy link
Member

Luap99 commented May 23, 2024

containers/common#1993 is merged. Waiting for renovate to update the containers/common dependency then this can me merged as well.

You can add second commit here that vendors c/common@main (needs to ensure the commit is before this one to ensure it builds correctly)

Done. I've run a go get github.com/containers/common@79d954c7766303997d7dea4340442855e143fc5b with the last commit from https://github.com/containers/common/ and created a separate commit for the changes therein. Two additional repositories got updated, I assume because they are dependencies.

Please help me verify if this is correct.

Yes this is correct and normal. The commit is missing theSigned-off-by line though.

Update the containers/common dependency to the latest main with the
needed changes in Podmansh.

Signed-off-by: phoenix <felix.niederwanger@suse.com>
Use the configuration parameters from the newly introduced
PodmanshConfig struct. This allows podmansh to be configured via
configuration files.

Signed-off-by: phoenix <felix.niederwanger@suse.com>
@grisu48
Copy link
Contributor Author

grisu48 commented May 23, 2024

Yes this is correct and normal.

Thank you.

The commit is missing theSigned-off-by line though.

Fixed.

@Luap99 Luap99 added the No New Tests Allow PR to proceed without adding regression tests label May 23, 2024
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

openshift-ci bot commented May 23, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grisu48, 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 /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 May 23, 2024
The new c/image version is returning a slightly new error message[1] so
make tests use the new one.

[1] containers/image#2408

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
@Luap99
Copy link
Member

Luap99 commented May 23, 2024

@grisu48 FYI I pushed another commit to make the e2e tests pass

@grisu48
Copy link
Contributor Author

grisu48 commented May 23, 2024

@grisu48 FYI I pushed another commit to make the e2e tests pass

Thanks, LGTM!

@mheon
Copy link
Member

mheon commented May 23, 2024

Let's get this landed.
/lgtm
/cherry-pick v5.1

@openshift-cherrypick-robot
Copy link
Collaborator

@mheon: once the present PR merges, I will cherry-pick it on top of v5.1 in a new PR and assign it to you.

In response to this:

Let's get this landed.
/lgtm
/cherry-pick v5.1

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-sigs/prow repository.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 23, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit e53b96c into containers:main May 23, 2024
88 of 89 checks passed
@openshift-cherrypick-robot
Copy link
Collaborator

@mheon: #22683 failed to apply on top of branch "v5.1":

Applying: Update containers/common to latest main
.git/rebase-apply/patch:260: trailing whitespace.
# 
.git/rebase-apply/patch:263: trailing whitespace.
# 
warning: 2 lines add whitespace errors.
Using index info to reconstruct a base tree...
M	go.mod
M	go.sum
M	vendor/modules.txt
Falling back to patching base and 3-way merge...
Auto-merging vendor/modules.txt
Auto-merging go.sum
CONFLICT (content): Merge conflict in go.sum
Auto-merging go.mod
CONFLICT (content): Merge conflict in go.mod
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Update containers/common to latest main
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

Let's get this landed.
/lgtm
/cherry-pick v5.1

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-sigs/prow repository.

@grisu48
Copy link
Contributor Author

grisu48 commented May 23, 2024

Thank you everyone 🥳

@grisu48 grisu48 deleted the podmansh_sh branch May 23, 2024 12:46
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. lgtm Indicates that a PR is ready to be merged. No New Tests Allow PR to proceed without adding regression tests release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants