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

chore(deps): py bump docker ^7.1.0, requests ^2.32.2 #115

Merged

Conversation

tarilabs
Copy link
Member

@tarilabs tarilabs commented May 30, 2024

Resolves https://github.com/kubeflow/model-registry/security/dependabot/10

Description

  1. bump requests ^2.32,
  2. at the same bump docker ^7.1.0 or else testcontainers would fail due to docker mgt error with requests.exceptions.InvalidURL: Not supported URL scheme http+docker; see more: https://github.com/psf/requests/issues/6707#issuecomment-2121103400

How Has This Been Tested?

Example1:
Linux, no specific settings for docker engine
Screenshot from 2024-05-30 08-59-51

Example2:
Mac OSX, required settings for Testcontainers (need special permission for Ryuk, documented)
image

Merge criteria:

  • The commits and have meaningful messages; the author will squash them after approval or will ask to merge with squash.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@ederign
Copy link
Member

ederign commented May 30, 2024

/lgtm

Copy link
Contributor

@isinyaaa isinyaaa left a comment

Choose a reason for hiding this comment

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

Thanks for the update! Left some nits as suggestions, also a heads up to improve accessibility is to put meaningful text on links.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated
Comment on lines 79 to 86
We recommend setting up the Podman machine with root priviledges,
and setting the environment variable

```sh
export TESTCONTAINERS_RYUK_PRIVILEGED=true
```

when running TestContainer-based Model Registry Python tests (for more information, see [here](https://pypi.org/project/testcontainers/#:~:text=TESTCONTAINERS_RYUK_PRIVILEGED)).
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC by exporting the env variable we're already following the recommended usage, so I'd rewrite the recommendation, mainly taking out the "and" (as it sounds like there is something additional to be done when using the Podman machine in that sense). Also, nit (typo).

Suggested change
We recommend setting up the Podman machine with root priviledges,
and setting the environment variable
```sh
export TESTCONTAINERS_RYUK_PRIVILEGED=true
```
when running TestContainer-based Model Registry Python tests (for more information, see [here](https://pypi.org/project/testcontainers/#:~:text=TESTCONTAINERS_RYUK_PRIVILEGED)).
We recommend using a [privileged Podman machine](https://pypi.org/project/testcontainers/#:~:text=TESTCONTAINERS_RYUK_PRIVILEGED)) when running TestContainer-based Model Registry Python tests.
You can do that by setting the following environment variable
```sh
export TESTCONTAINERS_RYUK_PRIVILEGED=true

Copy link
Member Author

Choose a reason for hiding this comment

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

In my understanding they're not the same thing, as this proposed correction would make it look like (since the "podman machine" is now linking to Ryuk container privileges).

In my understanding, one thing is the emulation used to run containers in general on macOsx, for podman that is the "podman machine"; another thing is the Ryuk container, used by TestContainers as a ~"garbage collector" which need to opt-in to this privileges.

I have podman machine as root by default,
but if I miss to specify the environment variable for ryuk specifically it won't work for Testcontainers.

In conclusion, to me it's indeed an "and".

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for the heads up!

Copy link
Member Author

Choose a reason for hiding this comment

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

happy to revise this text if we later find anything I've summarized above is not accurate/correct 👍

Signed-off-by: Matteo Mortari <[email protected]>

Co-authored-by: Isabella Basso <[email protected]>
@google-oss-prow google-oss-prow bot removed the lgtm label Jun 4, 2024
CONTRIBUTING.md Outdated Show resolved Hide resolved
Signed-off-by: Matteo Mortari <[email protected]>
Copy link
Contributor

@rareddy rareddy 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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: isinyaaa, rareddy

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

@google-oss-prow google-oss-prow bot merged commit e695960 into kubeflow:main Jun 4, 2024
10 checks passed
mzhl1111 pushed a commit to mzhl1111/model-registry that referenced this pull request Jul 1, 2024
* chore(deps): py bump docker ^7.1.0, requests ^2.32.2

Signed-off-by: Matteo Mortari <[email protected]>

* add Mac OSX specific instructions

Signed-off-by: Matteo Mortari <[email protected]>

* Update CONTRIBUTING.md

Signed-off-by: Matteo Mortari <[email protected]>

Co-authored-by: Isabella Basso <[email protected]>

* Update CONTRIBUTING.md

Signed-off-by: Matteo Mortari <[email protected]>

---------

Signed-off-by: Matteo Mortari <[email protected]>
Co-authored-by: Isabella Basso <[email protected]>
Signed-off-by: muzhouliu <[email protected]>
dhirajsb pushed a commit to dhirajsb/model-registry-kfp that referenced this pull request Jul 10, 2024
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.

4 participants