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

Adds exec command for devfile components with kube push target #3083

Merged
merged 6 commits into from
Jul 15, 2020

Conversation

mik-dass
Copy link
Contributor

@mik-dass mik-dass commented May 5, 2020

What type of PR is this?

/kind feature

What does does this PR do / why we need it:

Adds a odo exec command which executes a user given command for components with kube push targets.

Which issue(s) this PR fixes:

Fixes #2857

How to test changes / Special notes to the reviewer:

  • push a devfile component with a kube push target
  • execute odo exec -- ls <valid_command> and the valid command should be executed in the pod.

@openshift-ci-robot openshift-ci-robot 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/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation labels May 5, 2020
@mik-dass
Copy link
Contributor Author

mik-dass commented May 5, 2020

error: some steps failed:
  * could not run steps: step src failed: could not wait for build: the build src failed after 1m17s with reason DockerBuildFailed: Dockerfile build strategy has failed.
{"command":"git merge --no-ff ee37a0122275d1c08cc9eea924dc...ot something we can merge\n","time":"2020-05-05T13:35:29Z"}
{"component":"clonerefs","error":"one or more of the recor..."msg":"Failed to clone refs","time":"2020-05-05T13:35:29Z"}
subprocess exited with status 1
subprocess exited with status 1

/retest

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label May 6, 2020
@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. labels May 6, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label May 11, 2020
@codecov
Copy link

codecov bot commented May 11, 2020

Codecov Report

Merging #3083 into master will decrease coverage by 0.14%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3083      +/-   ##
==========================================
- Coverage   46.27%   46.13%   -0.15%     
==========================================
  Files         112      112              
  Lines       11386    11422      +36     
==========================================
  Hits         5269     5269              
- Misses       5608     5644      +36     
  Partials      509      509              
Impacted Files Coverage Δ
pkg/devfile/adapters/docker/component/adapter.go 58.13% <0.00%> (-6.38%) ⬇️
...g/devfile/adapters/kubernetes/component/adapter.go 30.66% <0.00%> (-2.08%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d69aab...d026b18. Read the comment docs.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label May 12, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label May 13, 2020
@mik-dass mik-dass force-pushed the exec_dev branch 3 times, most recently from 15bb2aa to 26bf3cb Compare June 10, 2020 07:45
@mik-dass mik-dass changed the title [WIP] Adds exec command for devfile components with kube push target Adds exec command for devfile components with kube push target Jun 10, 2020
@openshift-ci-robot openshift-ci-robot 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 Jun 10, 2020
return errors.Wrapf(err, "error while retrieving container for odo component %s", a.ComponentName)
}

runCommand, err := common.GetRunCommand(a.Devfile.Data, "")
Copy link
Contributor

@adisky adisky Jun 11, 2020

Choose a reason for hiding this comment

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

How we will handle here, if runCommand was passed as an odo flag --run-command during odo push?
Same problem i am getting for odo log

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can add a flag for that. But during the discussion, we decided to go with the default container and command for now and implement that later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any update on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But during the discussion, we decided to go with the default container and command for now and implement that later.

We had decided to use the default run container during the discussion.

namespace string

command []string
EnvSpecificInfo *envinfo.EnvSpecificInfo
Copy link
Contributor

Choose a reason for hiding this comment

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

why additionally using EnvSpecificInfo? we have it in context inside ComponentOptions?
(not specific for your PR, it is duplicate at other places as well)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@mik-dass mik-dass force-pushed the exec_dev branch 3 times, most recently from a8abe66 to be1505c Compare June 15, 2020 15:15
Copy link
Contributor

@adisky adisky left a comment

Choose a reason for hiding this comment

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

@mik-dass if i run odo exec outside context dir, It gives error ✗ open /home/**/devfile.yaml: no such file or directory. can we wrap this error to component not found, Please run odo exec from component directory or use odo create to create a component.

// ExecRecommendedCommandName is the recommended exec command name
const ExecRecommendedCommandName = "exec"

var execExample = ktemplates.Examples(` # Executes a command inside the component
Copy link
Contributor

Choose a reason for hiding this comment

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

can we give some examples here? currently it shows

Examples:
  # Executes a command inside the component
  odo exec

// Complete completes exec args
func (eo *ExecOptions) Complete(name string, cmd *cobra.Command, args []string) (err error) {
if cmd.ArgsLenAtDash() <= -1 {
return fmt.Errorf("no command was given for the exec command")
Copy link
Contributor

Choose a reason for hiding this comment

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

could we elaborate error message here
command not provided
please provide a command to execute, odo exec -- <command to be executed>

@mik-dass
Copy link
Contributor Author

mik-dass commented Jul 7, 2020

@adisky Fixed.

@@ -138,6 +138,7 @@ jobs:
- travis_wait make test-cmd-devfile-watch
- travis_wait make test-cmd-devfile-delete
- travis_wait make test-cmd-devfile-registry
- travis_wait make test-cmd-devfile-exec
Copy link
Contributor

Choose a reason for hiding this comment

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

update name for the test as well

@@ -165,6 +166,7 @@ jobs:
- travis_wait make test-cmd-docker-devfile-catalog
- travis_wait make test-cmd-docker-devfile-delete
- travis_wait make test-cmd-docker-devfile-url
- travis_wait make test-cmd-docker-devfile-exec
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@@ -196,5 +198,6 @@ jobs:
- travis_wait make test-cmd-devfile-watch
- travis_wait make test-cmd-devfile-push
- travis_wait make test-cmd-devfile-debug
- travis_wait make test-cmd-devfile-exec
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

if err != nil {
return errors.Wrapf(err, "unable to get pod for component %s", a.ComponentName)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we check running status for pod?

@mik-dass
Copy link
Contributor Author

mik-dass commented Jul 9, 2020

@adisky Fixed

@mik-dass
Copy link
Contributor Author

gosec -severity medium -confidence medium -exclude G304,G204 -quiet  ./...
golangci-lint run ./... --timeout 5m
make[1]: golangci-lint: Command not found

/retest

@adisky
Copy link
Contributor

adisky commented Jul 14, 2020

@mik-dass check the tests, other than it looks good to me
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jul 14, 2020
@mik-dass
Copy link
Contributor Author

error: unable to read URL "https://raw.githubusercontent.com/openshift/library/master/arch/x86_64/community/wildfly/imagestreams/wildfly-centos7.json", server reported 500 Internal Server Error, status code=500

/retest

@mik-dass
Copy link
Contributor Author

[odo]  ✗  Waiting for component to start [4m] [WARNING x5: FailedAttachVolume]
Deleting project: fhyfofhhol

/retest

@girishramnani
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: girishramnani

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

@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 Jul 15, 2020
@mik-dass
Copy link
Contributor Author

  * could not initialize namespace: could not create role binding for: rolebindings.rbac.authorization.k8s.io is forbidden: User "system:serviceaccount:ci:ci-operator" cannot create resource "rolebindings" in API group "rbac.authorization.k8s.io" in the namespace "ci-op-5h225d4z"

/retest

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

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.

odo exec command
6 participants