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

Add an odo run command to manually execute command during odo dev #6857

Merged

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Jun 1, 2023

What type of PR is this:

/kind feature

What does this PR do / why we need it:

Which issue(s) this PR fixes:

Fixes partially #6568

The points covered by this PR are:

  • there should be a command (odo run for example) that takes one argument that is the command name and executes the command defined in devfile
  • odo run assumes that odo dev is running in a separate terminal, if odo dev is not running (resources not present on the cluster) odo run errors out and tells the user to run odo dev first.
  • if the command is exec command and the container where the program needs to be started is not running, the command should be executed as a Job; for podman this can be running inside a pod/container (this case is not possible currently: odo dev includes all the containers defined in the Devfile).
  • if the command is exec and the container is already running, it should be just executed inside the running container
  • if command is apply it just applies what it needs to :-) kubernetes/openshift components get created, container component gets turned into Deployment is dedicatedPod: true
  • if command is composite that it performs all action based on the above rules

This PR does not completely covers #6568. Points not implemented/tested yet are:

  • odo run streams logs into the terminal and waits for the command to finish.
  • user can interrupt the command by pressing control-c; cleanup the resources.
  • the exit code of odo run command reflect the exit code of the last devfile command executed. If the last command was apply and it was successful it returns 0, if failed returns 1
  • Because odo dev is running, if Kubernetes/OpenShift resources are created with Apply commands, they should be deployed in "Dev" mode (to be cleanup when odo dev terminates) - make it clear it will be cleaned up.

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:

@netlify
Copy link

netlify bot commented Jun 1, 2023

Deploy Preview for odo-docusaurus-preview canceled.

Name Link
🔨 Latest commit 2ada482
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/6479cf88b66bb200072d72ef

@feloy feloy requested a review from rm3l June 1, 2023 09:06
@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 Jun 1, 2023
@openshift-ci openshift-ci bot requested review from anandrkskd and ritudes June 1, 2023 09:07
@odo-robot
Copy link

odo-robot bot commented Jun 1, 2023

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

@odo-robot
Copy link

odo-robot bot commented Jun 1, 2023

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

@odo-robot
Copy link

odo-robot bot commented Jun 1, 2023

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

@odo-robot
Copy link

odo-robot bot commented Jun 1, 2023

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

@odo-robot
Copy link

odo-robot bot commented Jun 1, 2023

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

@odo-robot
Copy link

odo-robot bot commented Jun 1, 2023

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

@odo-robot
Copy link

odo-robot bot commented Jun 1, 2023

OpenShift Tests on commit 77841ae finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Jun 1, 2023

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

@feloy feloy force-pushed the feature-6568/odo-run-command branch from 9a603f0 to 150f36d Compare June 1, 2023 15:55
@feloy feloy force-pushed the feature-6568/odo-run-command branch from 150f36d to 8c6f335 Compare June 1, 2023 17:54
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.

Added a few comments..

pkg/odo/cli/run/run.go Show resolved Hide resolved
pkg/dev/podmandev/run.go Outdated Show resolved Hide resolved
pkg/libdevfile/libdevfile.go Outdated Show resolved Hide resolved
pkg/podman/pods.go Outdated Show resolved Hide resolved
pkg/dev/common/run.go Outdated Show resolved Hide resolved
@feloy feloy force-pushed the feature-6568/odo-run-command branch from e1a87d9 to dc42555 Compare June 2, 2023 09:54
@feloy feloy force-pushed the feature-6568/odo-run-command branch from dc42555 to 2ada482 Compare June 2, 2023 11:16
@feloy feloy requested review from rm3l and removed request for anandrkskd and ritudes June 2, 2023 11:32
@feloy feloy closed this Jun 2, 2023
@feloy feloy reopened this Jun 2, 2023
@sonarcloud
Copy link

sonarcloud bot commented Jun 2, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

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
6.1% 6.1% Duplication

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 the changes - it works great so far!

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 2, 2023
@feloy
Copy link
Contributor Author

feloy commented Jun 2, 2023

/override windows-integration-test/Windows-test

 [FAILED] [206.381 seconds]
E2E Test starting with non-empty Directory add Binding [It] should verify developer workflow of using binding as env in innerloop
C:/Users/Administrator.ANSIBLE-TEST-VS/4064/tests/e2escenarios/e2e_test.go:462
[...]
[FAILED] Expected
      <*url.Error | 0xc0001090e0>: {
          Op: "Post",
          URL: "http://127.0.0.1:62941/api/newuser",
          Err: <*errors.errorString | 0xc00008a130>{s: "EOF"},
      }
  to be nil

@openshift-ci
Copy link

openshift-ci bot commented Jun 2, 2023

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

In response to this:

/override windows-integration-test/Windows-test

[FAILED] [206.381 seconds]
E2E Test starting with non-empty Directory add Binding [It] should verify developer workflow of using binding as env in innerloop
C:/Users/Administrator.ANSIBLE-TEST-VS/4064/tests/e2escenarios/e2e_test.go:462
[...]
[FAILED] Expected
     <*url.Error | 0xc0001090e0>: {
         Op: "Post",
         URL: "http://127.0.0.1:62941/api/newuser",
         Err: <*errors.errorString | 0xc00008a130>{s: "EOF"},
     }
 to be nil

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 Jun 2, 2023

/override SonarCloud Code Analysis
Duplicate code on mocks due to Platform interface embedded into Podman interface

@openshift-ci
Copy link

openshift-ci bot commented Jun 2, 2023

@feloy: /override requires failed status contexts, check run or a prowjob name to operate on.
The following unknown contexts/checkruns were given:

  • Analysis
  • Code
  • SonarCloud

Only the following failed contexts/checkruns were expected:

  • Kubernetes-Integration-Tests/Kubernetes-Integration-Tests
  • OpenShift-Integration-tests/OpenShift-Integration-tests
  • OpenShift-Integration-tests/OpenShift-Unauth-Integration-tests
  • SonarCloud Code Analysis
  • Unit-Tests/Unit-Tests
  • ci/prow/odo-ocp4.13-lp-interop-images
  • ci/prow/odo-ocp4.14-lp-interop-images
  • ci/prow/v4.10-images
  • ci/prow/v4.11-images
  • ci/prow/v4.12-images
  • ci/prow/v4.13-images
  • netlify/odo-docusaurus-preview/deploy-preview
  • pull-ci-redhat-developer-odo-main-odo-ocp4.13-lp-interop-images
  • pull-ci-redhat-developer-odo-main-odo-ocp4.14-lp-interop-images
  • pull-ci-redhat-developer-odo-main-v4.10-images
  • pull-ci-redhat-developer-odo-main-v4.11-images
  • pull-ci-redhat-developer-odo-main-v4.12-images
  • pull-ci-redhat-developer-odo-main-v4.13-images
  • tide
  • validator/Validate
  • windows-integration-test/Windows-test

If you are trying to override a checkrun that has a space in it, you must put a double quote on the context.

In response to this:

/override SonarCloud Code Analysis
Duplicate code on mocks due to Platform interface embedded into Podman interface

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 Jun 2, 2023

/override "SonarCloud Code Analysis"

@openshift-ci
Copy link

openshift-ci bot commented Jun 2, 2023

@feloy: Overrode contexts on behalf of feloy: SonarCloud Code Analysis

In response to this:

/override "SonarCloud Code Analysis"

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 330b724 into redhat-developer:main Jun 2, 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.

3 participants