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

Remove _catalog API, reference as reserved #69

Merged
merged 1 commit into from
Oct 21, 2019
Merged

Remove _catalog API, reference as reserved #69

merged 1 commit into from
Oct 21, 2019

Conversation

SteveLasker
Copy link
Contributor

@SteveLasker SteveLasker commented Aug 16, 2019

Amends: #45
This PR is the result of conversations on the dev alias and weekly online meetings (https://hackmd.io/El8Dd2xrTlCaCG59ns5cwg#Agenda-Items18)

  • _catalog
    • (lasker) there are registries that have not implemented it and will not.
    • So removing _catalog from the spec doesn't mean that folks will have to remove it from their registry, but rather that it will be easier for others to be compliant.

Signed-off-by: Steve Lasker StevenLasker@Hotmail.com

@mikebrow
Copy link
Member

Hey Steve... on the CI failure ... git-validation is very picky wrt what the commit header has to look like. I just use git commit -s -m "fixes issue ..." to sign vs trying to cut-n-paste to the commit header and get it right :-) Needs the empty line between the 80char or less commit description and before the signature.. example:

adds cni config data to the cri status/info

Signed-off-by: Mike Brown <brownwm@us.ibm.com>

# Please enter the commit message for your changes. Lines starting

@hallyn
Copy link
Contributor

hallyn commented Oct 6, 2019

Hi,

the commit message doesn't explain the reason for removing it. It's certainly a hacky thing, not well specified, and eventually will be a security issue, but especially until there's a querying api (or did I miss it's recent addition?) it's a useful hack :)

I assume my list of reasons for removing it is about right? :) But it would be good for the commit message to say so, or to correct me.

@SteveLasker
Copy link
Contributor Author

Hi @hallyn
I updated the PR info to reflect the previous conversations.
The new working group search/listing discussion moved from Hackmd and the KubeCon 2019 meeting notes, to a distribution-spec requirements discussion.
For the purposes of this PR, we just want to remove _catalog as a required implementation, that many registries won't implement, and leave it as reserved, so registries that did implement it won't get clobbered by a new api.

@hallyn
Copy link
Contributor

hallyn commented Oct 7, 2019

Oh! I misread the diff before, thought the 'optional' mention was being removed.

+1, thanks.

@vbatts
Copy link
Member

vbatts commented Oct 9, 2019

So this PR is not just removing _catalog, but now specifying a listing. Is that a stop gap until the search discussion is done?

@SteveLasker
Copy link
Contributor Author

The diff viewing is a bit confusing as the tag listing spec made references to _catalog rules, only addressing a superset, unique to tag listing.
Once I removed _catalog, I had to bring forward the specifics to paging and such.
Did we want to remove tag listing as well? I thought, but could be wrong, that most registries supported tag listing and it wasn't problematic.
That said, whatever the future replacement is, it would need to address repositories and tag listing.
Is this a vote to remove tag listing as well?

@vbatts
Copy link
Member

vbatts commented Oct 9, 2019 via email

@SteveLasker
Copy link
Contributor Author

This PR isn't about defining a new API. It's just clearing out inconsistently supported APIs so we can release a 1.0 of the distribution-spec. As written, the _catalog api isn't, and apparently won't be implemented by many registries. This is just removing it form the spec, so registries can be compliant with a 1.0 of the spec, but reserving _catalog so registries that did implement it won't get clobbered by a new api.

Copy link
Member

@mikebrow mikebrow 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
Member

@dmcgowan dmcgowan 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
Member

@jzelinskie jzelinskie left a comment

Choose a reason for hiding this comment

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

after this point, i'm in agreement with this change

the diff seems non-ideal, maybe a different git algorithm would fix it, but it's no big deal

The list of available repositories is made available through the _catalog_.
Repository listing is reserved for a future version of the distribution spec.

The `_catalog` api is reserved for historical usage. Registries MAY implement _catalog, but are NOT required.
Copy link
Member

Choose a reason for hiding this comment

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

Can we link to Docker v2 registry protocol spec in reference to this.
Because as of right now there is absolutely no context about what _catalog was

Copy link
Contributor Author

@SteveLasker SteveLasker Oct 11, 2019

Choose a reason for hiding this comment

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

We discussed on the call this week. Rather than have a pointer for someone to implement an old, inconsistently implemented spec, we agreed to focus on defining a new api. The intent here isn't to say here's a deprecated api. Rather, we're stating _catalog isn't part of the spec, however we do reserve it so a future version of the spec wouldn't attempt to use it. Thus, preserving any previous implementations.

https://hackmd.io/El8Dd2xrTlCaCG59ns5cwg#Notes1

  • General discussion on how to move along with the removal of the _catalog endpoint as a required part of the 1.0 distribution spec. It will be a reserved API route, but there will be no recommended (or optional) implementation of a Catalog API.
  • agreement from a few maintainers on the call to get reviews completed soon, no disagreement on the general direction and wording.

Copy link
Member

Choose a reason for hiding this comment

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

I think that people trying to understand pre-existing registries also read the spec, so I can at least see value linking to something in that use case. Either way, I'd rather drop this sooner than later, so this is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Jimmy,
Are you ok if we don’t reference it? You’ve got this mares as requesting the change.
I’m sure there are many places to get the info. Just keeping the spec as clean as possible.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with not including it because it keeps the spec more self contained and will only become more irrelevant over time.

Signed-off-by: Steve Lasker <stevenlasker@hotmail.com>
@jzelinskie
Copy link
Member

jzelinskie commented Oct 15, 2019

LGTM

Approved with PullApprove

@jzelinskie jzelinskie added this to the v1.0.0-rc1 milestone Oct 15, 2019
@SteveLasker
Copy link
Contributor Author

@mikebrow, can we merge this?

@vbatts
Copy link
Member

vbatts commented Oct 21, 2019

LGTM

Approved with PullApprove

@vbatts vbatts merged commit add4980 into opencontainers:master Oct 21, 2019
@mikebrow
Copy link
Member

@mikebrow, can we merge this?

thx for merging vince... missed the @ ping from Steve... something about my daughter having a daughter that week :)

@vbatts
Copy link
Member

vbatts commented Dec 16, 2019

:-D

@SteveLasker
Copy link
Contributor Author

@mikebrow wow, congrats. That's something to celebrate!

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.

6 participants