feat: suppport repo ls sub-namespace #768

wangxiaoxuan273 · 2023-01-28T09:42:25Z

Resolves #733

Test output on terminal:

Signed-off-by: wangxiaoxuan273 wangxiaoxuan119@gmail.com

wangxiaoxuan273 · 2023-01-28T10:05:03Z

I have two design questions:

When engineering is a repository and a namespace, should example-registry/engineering return the engineering repository?

i.e. :

$ oras repo ls example-registry
employeehandbook
engineering
engineering/bestpractices
engineering/newhires/onboarding
engineering/newhires/teamoverview

currently the engineering repository is not returned given example-registry/engineering.

$ oras repo ls example-registry/engineering
engineering/bestpractices
engineering/newhires/onboarding
engineering/newhires/teamoverview

Which display is better?

currently:

$ oras repo ls example-registry/engineering
engineering/bestpractices
engineering/newhires/onboarding
engineering/newhires/teamoverview

or

$ oras repo ls example-registry/engineering
bestpractices
newhires/onboarding
newhires/teamoverview

qweeah · 2023-01-28T13:08:45Z

@qweeah Please take a look at the failed e2e CI test. It's likely that the failed test need to be changed for this feature.

Yes you are right. One assertion can be removed since oras repo ls $REG/$REPO_SUFFIX is now a valid input.

qweeah · 2023-01-28T13:49:11Z

When engineering is a repository and a namespace, should example-registry/engineering return the engineering repository?

If a registry need to explicitly define a separate namespace concept from repository(e.g. ghcr, dockerhub), I don't think engineering can be a repository and a namespace at the same time.
If repository is only a slash separated path for a regsitry (e.g. ACR, MCR), engineering SHOULD be included in the result of oras repo ls example.registry/engineering. There is no need to hide the result of a full match.

qweeah · 2023-01-28T14:01:27Z

Which display is better?

I think the latter is out-of-option. Consider the scenario: How can the result of oras repo ls some.registry/my/repo be displayed if my/repo is a repository?

shizhMSFT · 2023-01-29T08:32:09Z

cmd/oras/repository/ls.go

+			hostname, namespace, _ := strings.Cut(args[0], "/")
+			opts.hostname = hostname
+			opts.namespace = namespace
+			// namespace is invalid if a tag is provided
+			if strings.Contains(namespace, ":") {
+				return fmt.Errorf("invalid namespace")
+			}


Can we refactor this part into a function so that we can unit test it?

Parsing can be simplified in an easier way.

If args[0] contains /, we can parse it using ParseReference(). Otherwise, it is a hostname.

Used ParseReference and created the function parseRepoPath to use in RunE.

cmd/oras/repository/ls.go

shizhMSFT · 2023-01-29T08:46:46Z

cmd/oras/repository/ls.go

+	before, after, found := strings.Cut(repopath, requestedNamespace)
+	if !found || after == "" || before != "" {
+		return false
+	}
+	return after[0] == '/'


It partially works and is not readable. Can we choose a simpler implementation?

The reason why it partially works is that the inNamespace("foo", "foo") returns false, which should be true. You need unit tests to ensure code quality.

It seems to be a UX issue other than a bug.

Used strings.HasPrefix(repopath, opts.namespace+"/") here after discussion.

codecov-commenter · 2023-01-29T09:04:00Z

Codecov Report

Merging #768 (ff3849e) into main (9c91686) will decrease coverage by 1.15%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #768      +/-   ##
==========================================
- Coverage   63.86%   62.71%   -1.15%     
==========================================
  Files          19       20       +1     
  Lines         714      802      +88     
==========================================
+ Hits          456      503      +47     
- Misses        225      257      +32     
- Partials       33       42       +9

Impacted Files	Coverage Δ
internal/graph/graph.go	`42.52% <0.00%> (ø)`
cmd/oras/internal/option/remote.go	`67.95% <0.00%> (+1.10%)`	⬆️
cmd/oras/internal/option/target.go	`21.68% <0.00%> (+1.68%)`	⬆️
cmd/oras/internal/option/spec.go	`17.24% <0.00%> (+17.24%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

shizhMSFT · 2023-01-29T09:23:26Z

I have two design questions:

When engineering is a repository and a namespace, should example-registry/engineering return the engineering repository?

i.e. :
$ oras repo ls example-registry
employeehandbook
engineering
engineering/bestpractices
engineering/newhires/onboarding
engineering/newhires/teamoverview
currently the engineering repository is not returned given example-registry/engineering.
$ oras repo ls example-registry/engineering
engineering/bestpractices
engineering/newhires/onboarding
engineering/newhires/teamoverview
Which display is better?

currently:
$ oras repo ls example-registry/engineering
engineering/bestpractices
engineering/newhires/onboarding
engineering/newhires/teamoverview
or
$ oras repo ls example-registry/engineering
bestpractices
newhires/onboarding
newhires/teamoverview

This is a good question. I would suggest the latter one:

$ oras repo ls example-registry/engineering
bestpractices
newhires/onboarding
newhires/teamoverview

In this way, it is more consistent with the UNIX ls command. In addition, it is more script-friendly that you can do

namespace=example-registry/engineering
oras repo ls $namespace | while read repo; do
    repo=$namespace/$repo
    echo Tags in $repo:
    oras repo tags $repo
    echo
done

FeynmanZhou · 2023-01-30T09:08:18Z

@wangxiaoxuan273 I vote for the latter one and agree with @shizhMSFT 's suggestion.

wangxiaoxuan273 · 2023-01-31T06:20:09Z

Which display is better?

I think the latter is out-of-option. Consider the scenario: How can the result of oras repo ls some.registry/my/repo be displayed if my/repo is a repository?

Then oras repo ls some.registry/my/repo does not return 'my/repo', if we choose the latter one.
Update: we've decided to use the latter display.

shizhMSFT · 2023-02-03T07:59:44Z

cmd/oras/repository/ls.go

@@ -60,15 +68,34 @@ Example - List the repositories under the registry that include values lexically
 	return cmd
 }

+func parseRepoPath(opts *repositoryOptions, arg string) error {


Can we add unit tests for this function?

Added unit tests.

shizhMSFT · 2023-02-03T08:00:31Z

cmd/oras/repository/ls.go

+			return err
+		}
+		opts.hostname = reference.Registry
+		opts.namespace = reference.Repository


Suggested change

opts.namespace = reference.Repository

opts.namespace = reference.Repository + "/"

so that we don't need to calculate it multiple time in a loop at line 96.

changed accordingly.

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

shizhMSFT

LGTM

shizhMSFT reviewed Jan 29, 2023

View reviewed changes

shizhMSFT requested a review from FeynmanZhou January 29, 2023 09:28

FeynmanZhou mentioned this pull request Jan 30, 2023

oras repo ls to support sub namespaces #733

Closed

1 task

wangxiaoxuan273 requested review from shizhMSFT and removed request for FeynmanZhou February 3, 2023 06:41

shizhMSFT reviewed Feb 3, 2023

View reviewed changes

wangxiaoxuan273 requested a review from shizhMSFT February 3, 2023 09:55

wangxiaoxuan273 added 3 commits February 3, 2023 11:00

feat: suppport repo ls sub-namespace

4af3aec

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

resolved comments with unit tests

d596f6c

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

updated go version

d8956cd

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

shizhMSFT approved these changes Feb 3, 2023

View reviewed changes

shizhMSFT merged commit 076d859 into oras-project:main Feb 3, 2023

wangxiaoxuan273 deleted the sub-namespaces branch February 3, 2023 11:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: suppport repo ls sub-namespace #768

feat: suppport repo ls sub-namespace #768

wangxiaoxuan273 commented Jan 28, 2023 •

edited

Loading

wangxiaoxuan273 commented Jan 28, 2023 •

edited

Loading

qweeah commented Jan 28, 2023

qweeah commented Jan 28, 2023

qweeah commented Jan 28, 2023

shizhMSFT Jan 29, 2023

shizhMSFT Jan 29, 2023

wangxiaoxuan273 Jan 31, 2023 •

edited

Loading

shizhMSFT Jan 29, 2023

shizhMSFT Jan 29, 2023

wangxiaoxuan273 Feb 2, 2023

codecov-commenter commented Jan 29, 2023 •

edited

Loading

shizhMSFT commented Jan 29, 2023

FeynmanZhou commented Jan 30, 2023

wangxiaoxuan273 commented Jan 31, 2023

shizhMSFT Feb 3, 2023

wangxiaoxuan273 Feb 3, 2023

shizhMSFT Feb 3, 2023

wangxiaoxuan273 Feb 3, 2023

shizhMSFT left a comment

	opts.namespace = reference.Repository
	opts.namespace = reference.Repository + "/"

feat: suppport repo ls sub-namespace #768

feat: suppport repo ls sub-namespace #768

Conversation

wangxiaoxuan273 commented Jan 28, 2023 • edited Loading

wangxiaoxuan273 commented Jan 28, 2023 • edited Loading

qweeah commented Jan 28, 2023

qweeah commented Jan 28, 2023

qweeah commented Jan 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangxiaoxuan273 Jan 31, 2023 • edited Loading

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

codecov-commenter commented Jan 29, 2023 • edited Loading

Codecov Report

shizhMSFT commented Jan 29, 2023

FeynmanZhou commented Jan 30, 2023

wangxiaoxuan273 commented Jan 31, 2023

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

shizhMSFT left a comment

Choose a reason for hiding this comment

wangxiaoxuan273 commented Jan 28, 2023 •

edited

Loading

wangxiaoxuan273 commented Jan 28, 2023 •

edited

Loading

wangxiaoxuan273 Jan 31, 2023 •

edited

Loading

codecov-commenter commented Jan 29, 2023 •

edited

Loading