Skip to content

fix(cmd): prevent adding connection with invalid identities file #26042

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

axel7083
Copy link
Contributor

@axel7083 axel7083 commented May 1, 2025

Fixes #26016

Does this PR introduce a user-facing change?

Ensure identities file exists when adding ssh connection

Fixes containers#26016

Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
@openshift-ci openshift-ci bot added release-note do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels May 1, 2025
Copy link
Contributor

openshift-ci bot commented May 1, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: axel7083
Once this PR has been reviewed and has the lgtm label, please assign ashley-cui for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

@mheon
Copy link
Member

mheon commented May 1, 2025

Code LGTM

Copy link
Member

@ashley-cui ashley-cui left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +147 to +150
case errors.Is(err, os.ErrNotExist):
logrus.Warnf("%q does not exist", entities.Identity)
case errors.Is(err, os.ErrPermission):
logrus.Warnf("You do not have permission to read %q", entities.Identity)
Copy link
Member

Choose a reason for hiding this comment

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

Why print a warning? Your commit says prevent adding but this doesn't prevent it at all.
IMO simply returning the error is the easiest and there is no need for us to special case each error, the orginal error should have enough information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(Windows): using podman system connection add with a ~ should convert it to absolute path
4 participants