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

Use new json connections file #374

Merged
merged 2 commits into from
Feb 21, 2024
Merged

Conversation

umohnani8
Copy link
Member

Podman has updated where it will store its
system connection information to a new json
format file.
Add support to podman-py to read from both the
new json file and old toml file giving preference
to the new json file.

Copy link
Member

@jwhonce jwhonce left a comment

Choose a reason for hiding this comment

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

One nit, a second test which verifies precedence on a duplicated connection name would be nice to have.

podman/domain/config.py Show resolved Hide resolved
@jwhonce jwhonce requested a review from Luap99 February 19, 2024 16:09
@umohnani8 umohnani8 force-pushed the 5.0-dev branch 3 times, most recently from 89bfff5 to 35b3643 Compare February 19, 2024 21:13
podman/domain/config.py Outdated Show resolved Hide resolved
@umohnani8 umohnani8 force-pushed the 5.0-dev branch 5 times, most recently from 51aec2f to bc2d540 Compare February 20, 2024 15:02
Podman has updated where it will store its
system connection information to a new json
format file.
Add support to podman-py to read from both the
new json file and old toml file giving preference
to the new json file.

Signed-off-by: Urvashi Mohnani <[email protected]>
Signed-off-by: Urvashi Mohnani <[email protected]>
@umohnani8
Copy link
Member Author

Tests are green!

@jwhonce PTAL

Copy link
Member

@jwhonce jwhonce left a comment

Choose a reason for hiding this comment

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

LGTM

podman/domain/config.py Show resolved Hide resolved
Copy link
Contributor

openshift-ci bot commented Feb 21, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jwhonce, umohnani8

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

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.

A couple of notes:
This is not honouring CONTAINERS_CONF or CONTAINERS_CONF or PODMAN_CONNECTIONS_CONF env overwrites.
It does not support reading containers.conf.d

I am not saying these have to be fixed here but I wanted to list it. I am fine to leave them unfixed until a user complains.

Comment on lines +76 to +80
# this elif is only for testing purposes
elif "@@is_test@@" in path:
test_path = path.replace("@@is_test@@", '')
self.path = Path(test_path) / "podman-connections.json"
old_toml_file = Path(test_path) / "containers.conf"
Copy link
Member

Choose a reason for hiding this comment

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

this sounds like an ugly hack but I am not a python programmer so I don't really care if this is the right thing to solve it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, I really dislike it too but I tried everything with the mock Env options but it just wouldn't pick up the env I was setting for XDG_CONFIG_HOME.
I can look into it further and open a PR in the future if I can figure it out.

@umohnani8
Copy link
Member Author

A couple of notes: This is not honouring CONTAINERS_CONF or CONTAINERS_CONF or PODMAN_CONNECTIONS_CONF env overwrites. It does not support reading containers.conf.d

I am not saying these have to be fixed here but I wanted to list it. I am fine to leave them unfixed until a user complains.

Agreed, I think it might be best to call podman system connection ls. Will have a discussion with @jwhonce on that and see what we can do with that. In the meantime let's get this in so we are somewhat up to date with the new connections file.

@umohnani8
Copy link
Member Author

@rhatdan @lsm5 can I please get a lgtm here

@Luap99
Copy link
Member

Luap99 commented Feb 21, 2024

Let's see if I have some power here
/lgtm

@openshift-ci openshift-ci bot added the lgtm label Feb 21, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 68f9296 into containers:main Feb 21, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants