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.

Sign up for GitHub

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

Use a single handler for executing all commands #6826

Merged
merged 33 commits into from
May 26, 2023
Merged

Use a single handler for executing all commands #6826

merged 33 commits into from
May 26, 2023

Conversation

feloy
Copy link
Contributor

@feloy feloy commented May 19, 2023

What type of PR is this:

/kind code-refactoring

What does this PR do / why we need it:

Which issue(s) this PR fixes:

Fixes #6792

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/code-refactoring labels May 19, 2023
@netlify
Copy link

netlify bot commented May 19, 2023 •
edited
Loading

✅ Deploy Preview for odo-docusaurus-preview canceled.

Name Link
🔨 Latest commit d22fc3a
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/6470b5fde69e9a0008c2353e

@openshift-ci openshift-ci bot requested review from anandrkskd and ritudes May 19, 2023 09:24
@odo-robot
Copy link

odo-robot bot commented May 19, 2023 •
edited
Loading

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

@odo-robot
Copy link

odo-robot bot commented May 19, 2023 •
edited
Loading

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

@odo-robot
Copy link

odo-robot bot commented May 19, 2023 •
edited
Loading

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

@odo-robot
Copy link

odo-robot bot commented May 19, 2023 •
edited
Loading

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

@odo-robot
Copy link

odo-robot bot commented May 19, 2023 •
edited
Loading

Windows Tests (OCP) on commit 7205e69 finished with errors.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented May 19, 2023 •
edited
Loading

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

@odo-robot
Copy link

odo-robot bot commented May 19, 2023 •
edited
Loading

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

@odo-robot
Copy link

odo-robot bot commented May 19, 2023 •
edited
Loading

Kubernetes Docs Tests on commit 511aaa2 finished successfully.
View logs: TXT HTML

@feloy feloy force-pushed the refacto-6792/single-handler branch 5 times, most recently from 04018b4 to 5dd0879 Compare May 22, 2023 14:06
@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 May 24, 2023
@feloy feloy force-pushed the refacto-6792/single-handler branch from 18ebc5a to e9c739b Compare May 24, 2023 09:39
@feloy feloy force-pushed the refacto-6792/single-handler branch from 166896c to 7429326 Compare May 24, 2023 14:28
@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 May 24, 2023
@feloy feloy force-pushed the refacto-6792/single-handler branch from b3e295c to ba1d715 Compare May 25, 2023 07:08
@feloy feloy changed the title [wip] Use a single handler for executing all commands Use a single handler for executing all commands May 25, 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 May 25, 2023
@feloy feloy requested review from rm3l and valaparthvi and removed request for anandrkskd and ritudes May 25, 2023 10:20
rm3l
rm3l reviewed May 26, 2023
View reviewed changes
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.

Thanks for your work on this! LGTM overall.
Had a few minor comments..

pkg/devfile/image/image.go Outdated
@@ -103,10 +107,10 @@ func buildPushImage(backend Backend, fs filesystem.Filesystem, image *devfile.Im
return nil
}

// selectBackend selects the container backend to use for building and pushing images
// SelectBackend selects the container backend to use for building and pushing images
// It will detect podman and docker CLIs (in this order),
// or return an error if none are present locally
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
// or return an error if none are present locally
// or return nil if none are present locally

pkg/component/handler_test.go Outdated
}

/*
func TestApplyImage(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.

Can we remove these commented lines if not needed? I can see they seem to be covered with the big test above..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to cleanup these old tests. Thanks

pkg/component/exec_handler_test.go Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this test file?

@feloy feloy requested a review from rm3l May 26, 2023 13:37
@sonarqubecloud
Copy link

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 14 Code Smells

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 May 26, 2023
@feloy
Copy link
Contributor Author

feloy commented May 26, 2023

/override

@feloy
Copy link
Contributor Author

feloy commented May 26, 2023

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

  [oc] Error from server (InternalError): error when creating "/go/odo_1/tests/examples/manifests/bindablekind-instance.yaml": Internal error occurred: failed calling webhook "mcluster.kb.io": failed to call webhook: Post "https://postgresql-operator-controller-manager-1-18-3-service.openshift-operators.svc:443/mutate-postgresql-k8s-enterprisedb-io-v1-cluster?timeout=10s": context deadline exceeded

@openshift-ci
Copy link

openshift-ci bot commented May 26, 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

 [oc] Error from server (InternalError): error when creating "/go/odo_1/tests/examples/manifests/bindablekind-instance.yaml": Internal error occurred: failed calling webhook "mcluster.kb.io": failed to call webhook: Post "https://postgresql-operator-controller-manager-1-18-3-service.openshift-operators.svc:443/mutate-postgresql-k8s-enterprisedb-io-v1-cluster?timeout=10s": context deadline exceeded

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.

@feloy
Copy link
Contributor Author

feloy commented May 26, 2023

/override windows-integration-test/Windows-test

  [oc] ''Unable to connect to the server: dial tcp 161.156.183.218:30329: connectex: A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond.

@openshift-ci
Copy link

openshift-ci bot commented May 26, 2023

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

In response to this:

/override windows-integration-test/Windows-test

 [oc] ''Unable to connect to the server: dial tcp 161.156.183.218:30329: connectex: A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond.

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 9a239c4 into redhat-developer:main May 26, 2023
@rm3l rm3l added the area/refactoring Issues or PRs related to code refactoring label Jun 19, 2023
@feloy feloy mentioned this pull request Aug 1, 2023
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rm3l rm3l rm3l approved these changes

@valaparthvi valaparthvi Awaiting requested review from valaparthvi

Assignees

@rm3l rm3l

Labels
area/refactoring Issues or PRs related to code refactoring lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use a single handler for executing a command on container / applying images/resources
3 participants
@feloy @rm3l @openshift-merge-robot