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

Jump to bottom

Bug 1576881 - Add discovery to openshift adapter #77

Merged
merged 4 commits into from May 15, 2018
Merged

Bug 1576881 - Add discovery to openshift adapter #77

merged 4 commits into from May 15, 2018

Conversation

dymurray
Copy link
Contributor

No description provided.

@dymurray dymurray mentioned this pull request May 10, 2018
Merged
jmrodri
jmrodri approved these changes May 11, 2018
View reviewed changes
Copy link
Contributor

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

ACK

@jwmatthews
Copy link
Member

I have some concerns with the naming of this adapter, I think it's misleading.

The support we added in this PR was for the partner side of the RHCC registry.
It is using a combination of "Openshift Registry" for images and a custom catalog v2 endpoint they implemented themselves, it's not a fair representation of an "OpenShift Registry".

If we are considering the specific image registry implemented provided by "OpenShift Registry" it does not support catalog v2 API.

There is an unrelated effort to implement an ability to list images in the actual OpenShift Registry being tracked in this card and planned for OCP 3.11: https://trello.com/c/AZINw5qI

I think we should consider a name change of this adapter to be clearer in it's intent, perhaps something like "partner_rhcc".

jwmatthews
jwmatthews requested changes May 11, 2018
View reviewed changes
Copy link
Member

@jwmatthews jwmatthews left a comment

Choose a reason for hiding this comment

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

Please re-evaluate the name of this adapter, I think calling it "openshift_adapter" is misleading.

@dymurray
Copy link
Contributor Author

@jwmatthews +1 to using partner_rhcc. Will make the change. Long term solution I think that we need to combine the different registry adapters which are implementing the docker registry API that is specified here into one adapter that can handle the different schema responses so that our naming is more consistent.

@dymurray
Copy link
Contributor Author

Waiting on review of these changes before I make them to #74

@johnkim76 johnkim76 mentioned this pull request May 15, 2018
Merged
jwmatthews
jwmatthews reviewed May 15, 2018
View reviewed changes
Copy link
Member

@jwmatthews jwmatthews left a comment

Choose a reason for hiding this comment

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

Name change looks good to me.

eriknelson
eriknelson approved these changes May 15, 2018
View reviewed changes
Copy link
Member

@eriknelson eriknelson left a comment

Choose a reason for hiding this comment

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

VISACK

registries/adapters/partner_rhcc_adapter_test.go
ocpa := OpenShiftAdapter{}
ocpa.Config.Images = Images
imagesFound, err := ocpa.GetImageNames()
func TestPartnerGetImageNames(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

👍

@shawn-hurley shawn-hurley merged commit d1bf04c into automationbroker:release-0.1 May 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmrodri jmrodri jmrodri approved these changes

@jwmatthews jwmatthews jwmatthews approved these changes

@eriknelson eriknelson eriknelson approved these changes

@shawn-hurley shawn-hurley shawn-hurley approved these changes

Assignees
No one assigned
Labels
0.1.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@dymurray @jwmatthews @jmrodri @eriknelson @shawn-hurley