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

Speedup odo registry --details #6678

Merged

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Mar 23, 2023

What type of PR is this:

/kind feature

What does this PR do / why we need it:

  • odo registry --details -o json continues to make new calls to get Devfile content for all stacks of registries (to not break plugin compatibility). This command will be deprecated, as --details flag was not intended to be used without --devfile flag.
  • odo registry --details --devfile "name" -o json continues to make a new call to get the "name" Devfile content (to not break plugin compatibility).
  • odo registry --details do not make anymore new calls to get Devfile content for all stacks of registries. This command will be deprecated, as --details flag was not intended to be used without --devfile flag.
  • odo registry --details --devfile "name" do not make anymore a new call to get the "name" Devfile content.

Which issue(s) this PR fixes:

Fixes #6401

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:

@openshift-ci openshift-ci bot added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Mar 23, 2023
@netlify
Copy link

netlify bot commented Mar 23, 2023

Deploy Preview for odo-docusaurus-preview ready!

Name Link
🔨 Latest commit c347db3
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/642ab7350c353b000836d42e
😎 Deploy Preview https://deploy-preview-6678--odo-docusaurus-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@openshift-ci openshift-ci bot requested review from rm3l and valaparthvi March 23, 2023 15:04
@odo-robot
Copy link

odo-robot bot commented Mar 23, 2023

NoCluster Tests on commit b4a4b10 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Mar 23, 2023

Unit Tests on commit b4a4b10 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Mar 23, 2023

Validate Tests on commit b4a4b10 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Mar 23, 2023

Windows Tests (OCP) on commit b4a4b10 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Mar 23, 2023

Kubernetes Tests on commit b4a4b10 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Mar 23, 2023

OpenShift Tests on commit b4a4b10 finished with errors.
View logs: TXT HTML

@feloy feloy changed the title Speedup odo registry --details [WIP] Speedup odo registry --details Mar 24, 2023
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Mar 24, 2023
Copy link
Contributor

@valaparthvi valaparthvi left a comment

Choose a reason for hiding this comment

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

Changes lgtm, and the speed is visible. One small change is to remove reference to devfileData from L132.

Copy link
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

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

/hold

A side-effet is that the stack content is not available anymore in the JSON output of the command. This removal will break the IDE plugins.

Indeed, I think we'll need to check with the IDE plugins or find a way not to break the output structure..

After a quick search into the source code, the VSCode plugin seems to rely on the .devfileData.devfile field from the odo registry --details output.
See:

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Mar 24, 2023
@feloy
Copy link
Contributor Author

feloy commented Mar 24, 2023

/hold

A side-effet is that the stack content is not available anymore in the JSON output of the command. This removal will break the IDE plugins.

Indeed, I think we'll need to check with the IDE plugins or find a way not to break the output structure..

After a quick search into the source code, the VSCode plugin seems to rely on the .devfileData.devfile field from the odo registry --details output. See:

Yes, right. These lines particularly are using the supportedOdoFeatures which are removed by the current patch.

https://github.com/redhat-developer/vscode-openshift-tools/blob/main/src/openshift/component.ts#L108-L110

Also the devfileData.devfile.metadata.name used in many places, for example
https://github.com/redhat-developer/vscode-openshift-tools/blob/main/src/openshift/component.ts#L386

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Mar 25, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Mar 28, 2023
@odo-robot
Copy link

odo-robot bot commented Mar 28, 2023

OpenShift Unauthenticated Tests on commit b4a4b10 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Mar 28, 2023

Kubernetes Docs Tests on commit bc0c023 finished with errors.
View logs: TXT HTML

@feloy
Copy link
Contributor Author

feloy commented Mar 28, 2023

@rm3l Here is my proposal to try and fix the issue about compatibility with plugins: 17bbda1

@feloy
Copy link
Contributor Author

feloy commented Mar 28, 2023

I've made changes according to what has been discussed during cabal. See #6678 (comment) for more details.

@feloy feloy changed the title [WIP] Speedup odo registry --details Speedup odo registry --details Mar 28, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Mar 28, 2023
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Mar 31, 2023
@feloy feloy requested a review from rm3l March 31, 2023 16:43
@feloy feloy closed this Apr 3, 2023
@feloy feloy reopened this Apr 3, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 3, 2023
@feloy
Copy link
Contributor Author

feloy commented Apr 3, 2023

/override windows-integration-test/Windows-test
Flaky test

@openshift-ci
Copy link

openshift-ci bot commented Apr 3, 2023

@feloy: Overrode contexts on behalf of feloy: windows-integration-test/Windows-test

In response to this:

/override windows-integration-test/Windows-test
Flaky test

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.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 3, 2023
@feloy feloy requested a review from rm3l April 3, 2023 10:58
@sonarcloud
Copy link

sonarcloud bot commented Apr 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 3, 2023
@rm3l
Copy link
Member

rm3l commented Apr 3, 2023

Sounds good to me to remove the hold on this PR, now that we are pretty confident in not breaking the IDE plugins, no? What do you think?
I added a reminder to document the deprecated odo registry --details (without --devfile) in the v3.9.0 release notes too.

@feloy
Copy link
Contributor Author

feloy commented Apr 3, 2023

Sounds good to me to remove the hold on this PR, now that we are pretty confident in not breaking the IDE plugins, no? What do you think? I added a reminder to document the deprecated odo registry --details (without --devfile) in the v3.9.0 release notes too.

Yes, the JSON output is not changed by this patch, it should not break the IDE plugins

@rm3l
Copy link
Member

rm3l commented Apr 3, 2023

Sounds good to me to remove the hold on this PR, now that we are pretty confident in not breaking the IDE plugins, no? What do you think? I added a reminder to document the deprecated odo registry --details (without --devfile) in the v3.9.0 release notes too.

Yes, the JSON output is not changed by this patch, it should not break the IDE plugins

Thanks for confirming.

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Apr 3, 2023
@feloy
Copy link
Contributor Author

feloy commented Apr 3, 2023

/override OpenShift-Integration-tests/OpenShift-Integration-tests

Flaky e2e test

 [FAILED] [212.271 seconds]
E2E Test starting with non-empty Directory add Binding [It] should verify developer workflow of using binding as env in innerloop
/go/odo_1/tests/e2escenarios/e2e_test.go:337

@openshift-ci
Copy link

openshift-ci bot commented Apr 3, 2023

@feloy: Overrode contexts on behalf of feloy: OpenShift-Integration-tests/OpenShift-Integration-tests

In response to this:

/override OpenShift-Integration-tests/OpenShift-Integration-tests

Flaky e2e test

[FAILED] [212.271 seconds]
E2E Test starting with non-empty Directory add Binding [It] should verify developer workflow of using binding as env in innerloop
/go/odo_1/tests/e2escenarios/e2e_test.go:337

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.

@openshift-merge-robot openshift-merge-robot merged commit 252a77d into redhat-developer:main Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Speedup odo registry --details
4 participants