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

Extend the parameters of 'images.load' and 'login' methods #426

Closed
wants to merge 17 commits into from

Conversation

milanbalazs
Copy link
Contributor

The new parameters of login method based on SystemAuth docs:

  • auth
  • identitytoken: IdentityToken is used to authenticate the user and get an access token for the registry.
  • registrytoken: RegistryToken is a bearer token to be sent to a registry
  • tls_verify: Whether to verify TLS certificates.

The new parameter of images.load method:

  • file_path: Path of the Tarball. If it's set then the load method opens the file and reads it as bytes. It means the method is able to handle file path as well, not only bytes. The scenarios when none of parameters or both of them are set are handled in the method.

Fix for:

Copy link
Contributor

openshift-ci bot commented Sep 12, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: milanbalazs
Once this PR has been reviewed and has the lgtm label, please assign vrothberg for approval. For more information see the Kubernetes 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

Signed-off-by: Milan Balazs <[email protected]>
Signed-off-by: Milan Balazs <[email protected]>
@milanbalazs
Copy link
Contributor Author

I think the test failure (time-out) is connected to this issue:

Copy link
Contributor

@inknos inknos left a comment

Choose a reason for hiding this comment

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

@milanbalazs, thanks for the PR. tests should now be working if you want to rebase it.

Overall looks good, but I'd like to wait for the comments I asked before going on

podman/domain/system.py Outdated Show resolved Hide resolved
podman/api/client.py Outdated Show resolved Hide resolved
podman/domain/system.py Show resolved Hide resolved
@milanbalazs
Copy link
Contributor Author

I don't really understand why the UnitTest says:

    @requests_mock.Mocker()
    def test_load(self, mock):
>       with self.assertRaises(PodmanError):
E       AssertionError: PodmanError not raised

Based on the change, it's clearly visible that the PodmanError should be raise in case of missing or too many arguments:

@inknos Do you have any idea?

podman/domain/images_manager.py Outdated Show resolved Hide resolved
podman/domain/system.py Show resolved Hide resolved
podman/domain/images_manager.py Show resolved Hide resolved
@inknos
Copy link
Contributor

inknos commented Sep 23, 2024

..oh wait, this project still supports Python <3.8, so it's a no-go. Anyway, leaving the comment.

BTW, currently I am fighting with the PyLint in your gate

Hopefully will figure out these things in this future PR #422

Copy link
Contributor

@inknos inknos left a comment

Choose a reason for hiding this comment

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

@milanbalazs
Please address the small issue regarding the generator, rebase, squash it to fewer commits to have the logic of the changes a bit more clear, and we are ready to go :)

Thanks again for your PR and for reading all my comments, I understand it takes a lot of time, but I am happy with the result

podman/domain/images_manager.py Outdated Show resolved Hide resolved
@inknos
Copy link
Contributor

inknos commented Sep 25, 2024

PR looks good now. I'd like to clean up the commits a bit and rebase. I made a test branch with just a logical squash of the commits in 2. Then I rebased it with the upstrea/main.

If you are ok with it, @milanbalazs , I would like to push it to this branch. It's just polishing, there is absolutely no changes in your code.

https://github.com/inknos/podman-py/tree/squash-pr-upstream-426

(for a clean look you could also try previewing a PR to see how it would look.)

Once again, it's just a squash of the many commits, if you don't mind.

@milanbalazs
Copy link
Contributor Author

PR looks good now. I'd like to clean up the commits a bit and rebase. I made a test branch with just a logical squash of the commits in 2. Then I rebased it with the upstrea/main.

If you are ok with it, @milanbalazs , I would like to push it to this branch. It's just polishing, there is absolutely no changes in your code.

https://github.com/inknos/podman-py/tree/squash-pr-upstream-426

(for a clean look you could also try previewing a PR to see how it would look.)

Once again, it's just a squash of the many commits, if you don't mind.

@inknos It's absolutely OK from my side! :)

@inknos
Copy link
Contributor

inknos commented Sep 25, 2024

Thanks @milanbalazs. I noticed that you created this PR from your main branch. Unfortunately I cannot edit it (maybe I can but I don't want to mess up with your main branch) and it is good practive to make a PR from a branch.

You could open a new PR with the fixes, link it here and about this one, "close with comment". Then I'll pass the new one.

Here are the steps to achieve it quickly if you need them.

# add the remote with the squashed commits
git remote add inknos https://github.com/inknos/podman-py.git 
# clone my branch
git fetch inknos refs/heads/squash-pr-upstream-426 
# optional: checkout to it
git checkout squash-pr-upstream-426
# optional: check what's there
git log -p
# push it to your GH and open a new PR
git push origin squash-pr-upstream-426
# optional: remove the remote and the branch once it's done
git switch main ; git branch -D squash-pr-upstream-426 ; git remote remove inknos

Thanks :)

@milanbalazs
Copy link
Contributor Author

@inknos Thanks for your support! Here is the new PR: #434

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants