core: align config_path type annotation #166

tarilabs · 2024-10-20T09:11:17Z

ref #164 (comment)

will add explanation on this proposal after raising the PR

Signed-off-by: tarilabs <[email protected]>

the oras-py CI setup uses basic auth for auth tests Signed-off-by: tarilabs <[email protected]>

This reverts commit 2679ff8. Signed-off-by: tarilabs <[email protected]>

This reverts commit 2089de9. Signed-off-by: tarilabs <[email protected]>

Signed-off-by: tarilabs <[email protected]>

tarilabs

This is my proposal minimizing changes.

My understanding is that config_path on provider.py methods is intended to specify a custom docker config_path where to write the credentials to on successful login, and to lookup the credentials from on push/pull.

On separate note, my understanding is that also utils.py load_configs is an utility function to load auths from multiple places, ie the default homedir docker config and a custom list of supplied paths.

tarilabs · 2024-10-20T09:13:50Z

oras/provider.py

-        config_path: Optional[List[str]] = None,
+        config_path: Optional[str] = None,


the config_path in login() is used to write the credentials to that config_path, after successful login

oras-py/oras/provider.py

Lines 196 to 205 in 6df6127

dockercfg_path=config_path,

)

# Fallback to manual login

except Exception:

return login.DockerClient().login(

username=username, # type: ignore

password=password, # type: ignore

registry=hostname, # type: ignore

dockercfg_path=config_path,

so I intend this is a "custom file" I want to use,
hence not an optional list,
but an optional str to keep consistency.

tarilabs · 2024-10-20T09:14:05Z

oras/provider.py

-        :param config_path: list of config paths to add
-        :type config_path: list
+        :param config_path: custom config path to add credentials to
+        :type config_path: str


align pydoc to above comment.

tarilabs · 2024-10-20T09:17:00Z

oras/provider.py

-        self.auth.load_configs(container, configs=config_path)
+        self.auth.load_configs(
+            container, configs=None if config_path is None else [config_path]
+        )


this makes an internal call to

oras-py/oras/auth/base.py

Line 86 in 6df6127

def load_configs(self, container: container_type, configs: Optional[list] = None):

in turn,

oras-py/oras/auth/utils.py

Line 14 in 6df6127

def load_configs(configs: Optional[List[str]] = None):

that has capabilities to load credentials from multiple files.

So, if the user has specified its own custom docker config_path, we pass it, otherwise None.

tarilabs · 2024-10-20T09:17:15Z

oras/provider.py

-        self.auth.load_configs(container, configs=config_path)
+        self.auth.load_configs(
+            container, configs=None if config_path is None else [config_path]
+        )


analogous as above comment, for pull

Ditto here to simplify the statement.

tarilabs · 2024-10-20T09:17:30Z

oras/tests/test_oras.py

+@pytest.mark.with_auth(True)
+def test_custom_docker_config_path(tmp_path, registry, credentials, target_dir):


this provides a test case

tarilabs · 2024-10-20T09:18:04Z

oras/tests/test_oras.py

+        tls_verify=False,
+        username=credentials.user,
+        password=credentials.password,
+        config_path=my_dockercfg_path,  # <-- for login


see first comment; here we specify a custom docker config_path where the credentials are written to on successful login.

tarilabs · 2024-10-20T09:18:21Z

oras/tests/test_oras.py

+    # Test push/pull with custom docker config_path
+    upload_dir = os.path.join(here, "upload_data")
+    res = client.push(
+        files=[upload_dir], target=target_dir, config_path=my_dockercfg_path


so we use the custom docker config_path during push,

tarilabs · 2024-10-20T09:18:28Z

oras/tests/test_oras.py

+    files = client.pull(
+        target=target_dir, outdir=tmp_path, config_path=my_dockercfg_path
+    )


... and pull.

vsoch

I like this design better @tarilabs ! I forget why we originally did a list - I do remember there was a reason and even at the time I thought it would be more likely to have just one config. Just a few tweaks to the statement I pointed out and this should be good.

vsoch · 2024-10-20T17:50:55Z

oras/provider.py

@@ -729,7 +729,9 @@ def push(
        """
        container = self.get_container(target)
        files = files or []
-        self.auth.load_configs(container, configs=config_path)
+        self.auth.load_configs(
+            container, configs=None if config_path is None else [config_path]


You could probably do [config_path] if config_path else None

vsoch · 2024-10-20T17:51:26Z

oras/provider.py

-        self.auth.load_configs(container, configs=config_path)
+        self.auth.load_configs(
+            container, configs=None if config_path is None else [config_path]
+        )


Ditto here to simplify the statement.

Signed-off-by: tarilabs <[email protected]> Co-authored-by: vsoch <[email protected]>

tarilabs · 2024-10-21T07:27:10Z

Just a few tweaks to the statement I pointed out and this should be good.

Make sense, I wanted to highlight the "most likely case" (ie no custom config_path) but I agree is much more readable this way; thanks for the review!

Signed-off-by: tarilabs <[email protected]>

vsoch

Looks great, thanks @tarilabs !

tarilabs · 2024-10-21T17:38:25Z

thanks @vsoch ! 🚀

tarilabs added 8 commits October 20, 2024 10:47

core: align config_path type annotation

3fa149d

Signed-off-by: tarilabs <[email protected]>

tmp

2679ff8

Signed-off-by: tarilabs <[email protected]>

keep

e0e89f4

Signed-off-by: tarilabs <[email protected]>

tmp

2089de9

Signed-off-by: tarilabs <[email protected]>

keep: test use basic

4cf74df

the oras-py CI setup uses basic auth for auth tests Signed-off-by: tarilabs <[email protected]>

Revert "tmp"

27a1000

This reverts commit 2679ff8. Signed-off-by: tarilabs <[email protected]>

Revert "tmp"

cbab8a5

This reverts commit 2089de9. Signed-off-by: tarilabs <[email protected]>

cleanups

8f59ff1

Signed-off-by: tarilabs <[email protected]>

tarilabs requested review from vsoch and SteveLasker as code owners October 20, 2024 09:11

tarilabs commented Oct 20, 2024

View reviewed changes

tarilabs mentioned this pull request Oct 20, 2024

Issue with accessing a private JFrog registry #164

Open

vsoch reviewed Oct 20, 2024

View reviewed changes

implement review feedback

939f723

Signed-off-by: tarilabs <[email protected]> Co-authored-by: vsoch <[email protected]>

tarilabs requested a review from vsoch October 21, 2024 07:28

bump version and maintain changelog entry

642c7bc

Signed-off-by: tarilabs <[email protected]>

vsoch approved these changes Oct 21, 2024

View reviewed changes

vsoch merged commit ff9c2e2 into oras-project:main Oct 21, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: align config_path type annotation #166

core: align config_path type annotation #166

tarilabs commented Oct 20, 2024

tarilabs left a comment

tarilabs Oct 20, 2024

tarilabs Oct 20, 2024

tarilabs Oct 20, 2024

tarilabs Oct 20, 2024

vsoch Oct 20, 2024

tarilabs Oct 20, 2024

tarilabs Oct 20, 2024

tarilabs Oct 20, 2024

tarilabs Oct 20, 2024

vsoch left a comment

vsoch Oct 20, 2024

vsoch Oct 20, 2024

tarilabs commented Oct 21, 2024

vsoch left a comment

tarilabs commented Oct 21, 2024

		config_path: Optional[List[str]] = None,
		config_path: Optional[str] = None,

	dockercfg_path=config_path,
	)

	# Fallback to manual login
	except Exception:
	return login.DockerClient().login(
	username=username, # type: ignore
	password=password, # type: ignore
	registry=hostname, # type: ignore
	dockercfg_path=config_path,

		@pytest.mark.with_auth(True)
		def test_custom_docker_config_path(tmp_path, registry, credentials, target_dir):

core: align config_path type annotation #166

core: align config_path type annotation #166

Conversation

tarilabs commented Oct 20, 2024

tarilabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsoch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarilabs commented Oct 21, 2024

vsoch left a comment

Choose a reason for hiding this comment

tarilabs commented Oct 21, 2024