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

OCI-based devifle registry support #4525

Merged
merged 16 commits into from
Apr 15, 2021

Conversation

GeekArthur
Copy link
Contributor

@GeekArthur GeekArthur commented Mar 18, 2021

Signed-off-by: jingfu wang [email protected]

What type of PR is this?
/kind feature

What does this PR do / why we need it:
This PR implements the OCI-based devfile registry support, more specifically:

  1. Consume devfile registry library or REST API (list devfile components + create devfile component)
  2. Change the default devfile registry URL to OCI-based public devfile registry

Which issue(s) this PR fixes:

Related issues:

  1. odo Migration to OCI based public community devfile registry devfile/api#367
  2. Use official devfile registry #4504

PR acceptance criteria:

How to test changes / Special notes to the reviewer:

  1. Run odo registry add OCI https://registry.devfile.io to add OCI-based public devfile registry if it's not in the registry list
  2. Run odo catalog list components then verify the devfiles from OCI-based public devfile registry can be listed
  3. Run odo create nodejs --registry OCI then verify the nodejs devfile can be downloaded from OCI-based public devfile registry

@GeekArthur GeekArthur self-assigned this Mar 18, 2021
@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Mar 18, 2021
@GeekArthur
Copy link
Contributor Author

/retest

2 similar comments
@GeekArthur
Copy link
Contributor Author

/retest

@GeekArthur
Copy link
Contributor Author

/retest

@GeekArthur
Copy link
Contributor Author

/retest

2 similar comments
@GeekArthur
Copy link
Contributor Author

/retest

@GeekArthur
Copy link
Contributor Author

/retest

@GeekArthur
Copy link
Contributor Author

/retest

@GeekArthur
Copy link
Contributor Author

/retest

5 similar comments
@GeekArthur
Copy link
Contributor Author

/retest

@GeekArthur
Copy link
Contributor Author

/retest

@GeekArthur
Copy link
Contributor Author

/retest

@GeekArthur
Copy link
Contributor Author

/retest

@GeekArthur
Copy link
Contributor Author

/retest

@kadel
Copy link
Member

kadel commented Mar 31, 2021

We need to find a way how to change DefaultDevfileRegistry for the current users.
Existing odo users will already have the existing DefaultDevfileRegistry record in preference.yaml, odo needs to update it to the new registry, otherwise, all current users will get stuck with the default GitHub based registry.

@johnmcollier
Copy link
Member

We need to find a way how to change DefaultDevfileRegistry for the current users.
Existing odo users will already have the existing DefaultDevfileRegistry record in preference.yaml, odo needs to update it to the new registry, otherwise, all current users will get stuck with the default GitHub based registry.

Yup, agreed. Some useful discussion in #3298 about this as well that might provide some insight on how we can do this

@johnmcollier
Copy link
Member

johnmcollier commented Mar 31, 2021

@GeekArthur The odo catalog describe tests are failing due to this error:

 [odo] Starter Projects:
[odo] I0330 15:04:12.105604   59639 validate.go:50] Successfully validated devfile sections
[odo] ---
[odo] name: nodejs-starter
[odo] attributes: {}
[odo] description: ""
[odo] subdir: ""
[odo] projectsource:
[odo]   sourcetype: ""
[odo]   git:
[odo]     gitlikeprojectsource:
[odo]       commonprojectsource: {}
[odo]       checkoutfrom: null
[odo]       remotes:
[odo]         origin: https://github.com/odo-devfiles/nodejs-ex.git
[odo]   github: null
[odo]   zip: null
[odo]   custom: null
[odo] 
[odo] * Registry: DefaultDevfileRegistry
[odo]  ✗  Get https://registry.devfile.iodevfile-catalog/nodejs:latest: dial tcp: lookup registry.devfile.iodevfile-catalog on 172.30.0.10:53: no such host 

You need to update the GetDevfile function to properly handle cases where the devfile is in an OCI registry (specifically the call to devfile.ParseFromURLAndValidate):

https://github.com/openshift/odo/blob/12b03e5924b1db8ea1634ecf22e9aab215bc7ab5/pkg/odo/cli/catalog/describe/component.go#L197-L209

@GeekArthur
Copy link
Contributor Author

We need to find a way how to change DefaultDevfileRegistry for the current users.
Existing odo users will already have the existing DefaultDevfileRegistry record in preference.yaml, odo needs to update it to the new registry, otherwise, all current users will get stuck with the default GitHub based registry.

Yup, agreed. Some useful discussion in #3298 about this as well that might provide some insight on how we can do this

Yeah agree, I also wanna link Maysun's issue here, thanks John.

@GeekArthur
Copy link
Contributor Author

@GeekArthur The odo catalog describe tests are failing due to this error:

 [odo] Starter Projects:
[odo] I0330 15:04:12.105604   59639 validate.go:50] Successfully validated devfile sections
[odo] ---
[odo] name: nodejs-starter
[odo] attributes: {}
[odo] description: ""
[odo] subdir: ""
[odo] projectsource:
[odo]   sourcetype: ""
[odo]   git:
[odo]     gitlikeprojectsource:
[odo]       commonprojectsource: {}
[odo]       checkoutfrom: null
[odo]       remotes:
[odo]         origin: https://github.com/odo-devfiles/nodejs-ex.git
[odo]   github: null
[odo]   zip: null
[odo]   custom: null
[odo] 
[odo] * Registry: DefaultDevfileRegistry
[odo]  ✗  Get https://registry.devfile.iodevfile-catalog/nodejs:latest: dial tcp: lookup registry.devfile.iodevfile-catalog on 172.30.0.10:53: no such host 

You need to update the GetDevfile function to properly handle cases where the devfile is in an OCI registry (specifically the call to devfile.ParseFromURLAndValidate):

https://github.com/openshift/odo/blob/12b03e5924b1db8ea1634ecf22e9aab215bc7ab5/pkg/odo/cli/catalog/describe/component.go#L197-L209

Yep, will fix that with other test automation failures.

@elsony
Copy link

elsony commented Apr 1, 2021

@GeekArthur , @kadel and I have discussed the default registry migration. We'll just take a look at the existing preference, if there is a default entry that points to the old repo, we'll just migrate the registry URL to the new URL silently in the preference file without prompting the user.

Signed-off-by: jingfu wang <[email protected]>
Signed-off-by: jingfu wang <[email protected]>
@GeekArthur
Copy link
Contributor Author

/retest

3 similar comments
@GeekArthur
Copy link
Contributor Author

/retest

@GeekArthur
Copy link
Contributor Author

/retest

@GeekArthur
Copy link
Contributor Author

/retest

@girishramnani
Copy link
Contributor

Is this a genuine failure?

@GeekArthur
Copy link
Contributor Author

Is this a genuine failure?

@girishramnani No, it's test infra issue, I posted it on odo slack channel: https://kubernetes.slack.com/archives/C01D6L2NUAG/p1618243057007700

@GeekArthur
Copy link
Contributor Author

@johnmcollier @kadel All comments addressed, I already discussed with Tomas, due to the test infra issues, the PR can be merged once it satisfies the following requirements:

  1. Has approve and lgtm labels
  2. Pass unit and ci/prow/v4.7-integration-e2e tests

Please review again, thanks.

Copy link
Member

@johnmcollier johnmcollier left a comment

Choose a reason for hiding this comment

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

No longer a reviewer on this repo, but approving from the devfile side of things

/lgtm

Edit: Didn't expect that to actually add the label 😅 , thought you had to be in the OWNERS file?

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 14, 2021
@kadel
Copy link
Member

kadel commented Apr 15, 2021

Edit: Didn't expect that to actually add the label 😅 , thought you had to be in the OWNERS file?

wow. Interesting. You still have to write permission on this repo, that might be why. But this is new to me :-)

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Apr 15, 2021
@kadel
Copy link
Member

kadel commented Apr 15, 2021

/approve

I have one small issue with the current migration approach. But that can be addressed separately.
Opened #4626 to track it

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Apr 15, 2021
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 15, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Apr 15, 2021
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: GeekArthur, kadel

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

@kadel
Copy link
Member

kadel commented Apr 15, 2021

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 15, 2021
@openshift-ci
Copy link

openshift-ci bot commented Apr 15, 2021

@GeekArthur: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/psi-openshift-integration-e2e b15145e link /test psi-openshift-integration-e2e
ci/prow/psi-minishift-integration-e2e b15145e link /test psi-minishift-integration-e2e

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@GeekArthur
Copy link
Contributor Author

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants