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 run-command flag to odo dev to run non-default Run command #5878

Conversation

rm3l
Copy link
Member

@rm3l rm3l commented Jun 27, 2022

What type of PR is this:
/kind feature
/area dev

What does this PR do / why we need it:
See #5775 for the use case.

Which issue(s) this PR fixes:
Fixes #5775

PR acceptance criteria:

How to test changes / Special notes to the reviewer:
odo dev --run-command <command> should allow to run the specified <command>, provided it is in the run group.

@netlify
Copy link

netlify bot commented Jun 27, 2022

Deploy Preview for odo-docusaurus-preview ready!

Name Link
🔨 Latest commit 068d864
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/62ba08573a90a700091d475e
😎 Deploy Preview https://deploy-preview-5878--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 added kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation area/dev Issues or PRs related to `odo dev` labels Jun 27, 2022
@openshift-ci openshift-ci bot requested a review from cdrage June 27, 2022 05:18
@odo-robot
Copy link

odo-robot bot commented Jun 27, 2022

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

@odo-robot
Copy link

odo-robot bot commented Jun 27, 2022

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

@odo-robot
Copy link

odo-robot bot commented Jun 27, 2022

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

@odo-robot
Copy link

odo-robot bot commented Jun 27, 2022

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

@odo-robot
Copy link

odo-robot bot commented Jun 27, 2022

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

tests/integration/devfile/cmd_dev_test.go Show resolved Hide resolved
cmdName string,
kind v1alpha2.CommandGroupKind,
handler Handler,
ignoreCommandNotFound bool,
Copy link
Member

Choose a reason for hiding this comment

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

In what case this boolean would be useful? What if user provides a custom command of certain kind and has set this boolean to true? Could you also brief this in function's comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

The use case for this was mainly for the Build command. The previous behavior I noticed was ignoring errors if the Devfile does not have any Build commands.
I'll update the description of this function - thanks.

commands, err := data.GetCommands(common.DevfileOptions{})
// getDefaultCommand iterates through the devfile commands and returns the default command associated with the group kind.
// If there is no default command, the second return value is false.
func getDefaultCommand(devfileObj parser.DevfileObj, groupType v1alpha2.CommandGroupKind) (v1alpha2.Command, bool, error) {
Copy link
Member

Choose a reason for hiding this comment

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

A plural for default command? Is it possible to have multiple default commands for the same group kind in a devfile?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, OK. It's an error to have multiple default commands. Isn't this already covered in the devfile validator? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Where is this Devfile validator? And when is it called in odo? I basically used what was done before, but if it is already validated elsewhere, we can remove this check here.

Copy link
Member

Choose a reason for hiding this comment

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

Where is this Devfile validator? And when is it called in odo?

func parseDevfile(args parser.ParserArgs) (parser.DevfileObj, error) {
devObj, varWarnings, err := devfile.ParseDevfileAndValidate(args)
if err != nil {
return parser.DevfileObj{}, err
}

I basically used what was done before, but if it is already validated elsewhere, we can remove this check here.

@feloy came up with libdevfile package as a way to add things on top of devfile library. These could eventually be put into the library itself. But that's a different story. Point is, you don't need to remove a check unless you can find a counterpart in devfile library.

pkg/libdevfile/libdevfile.go Outdated Show resolved Hide resolved
pkg/libdevfile/libdevfile.go Outdated Show resolved Hide resolved
@rm3l rm3l requested a review from dharmit June 27, 2022 10:32
@dharmit
Copy link
Member

dharmit commented Jun 27, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 27, 2022
pkg/devfile/adapters/kubernetes/utils/utils.go Outdated Show resolved Hide resolved
tests/helper/helper_dev.go Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 27, 2022
@rm3l rm3l requested a review from feloy June 27, 2022 16:01
@feloy
Copy link
Contributor

feloy commented Jun 27, 2022

/approve

@openshift-ci
Copy link

openshift-ci bot commented Jun 27, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feloy

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 openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Jun 27, 2022
rm3l added 5 commits June 27, 2022 21:41
Few functions used to have similar but slightly different logic,
which could make them quite confusing to use.
This also fixes a behavior which is not supposed to be
part of the Devfile spec: in the past, if a Devfile contains a single
but non-default command for a given kind, this command would get returned
as the Default command for such kind.
Now, an explicit error is returned instead if this single command
is not marked as default in the Devfile.
@rm3l rm3l force-pushed the 5775-odo-dev-add-flag-to-run-non-default-run-command branch from 978f52a to 068d864 Compare June 27, 2022 19:43
@sonarcloud
Copy link

sonarcloud bot commented Jun 27, 2022

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

@feloy
Copy link
Contributor

feloy commented Jun 28, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 28, 2022
@valaparthvi
Copy link
Contributor

/retest-required

@rm3l
Copy link
Member Author

rm3l commented Jun 28, 2022

/test v4.10-integration-e2e

@rm3l
Copy link
Member Author

rm3l commented Jun 28, 2022

/override ci/prow/v4.10-integration-e2e

Doesn't pass in Prow due to the same issues with SBO. Overriding because IBM Cloud tests are passing.

@openshift-ci
Copy link

openshift-ci bot commented Jun 28, 2022

@rm3l: Overrode contexts on behalf of rm3l: ci/prow/v4.10-integration-e2e

In response to this:

/override ci/prow/v4.10-integration-e2e

Doesn't pass in Prow due to the same issues with SBO. Overriding because IBM Cloud tests are passing.

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.

@rm3l
Copy link
Member Author

rm3l commented Jun 28, 2022

golangci/golangci-lint info checking GitHub for tag 'v1.37.0'
golangci/golangci-lint crit unable to find 'v1.37.0' - use 'latest' or see https://github.com/golangci/golangci-lint/releases for details
make[1]: *** [goget-tools] Error 1
make[1]: Leaving directory `/go/src/github.com/redhat-developer/odo'

/override ci/prow/unit

Unit tests passing in IBM Cloud.

@openshift-ci
Copy link

openshift-ci bot commented Jun 28, 2022

@rm3l: Overrode contexts on behalf of rm3l: ci/prow/unit

In response to this:

golangci/golangci-lint info checking GitHub for tag 'v1.37.0'
golangci/golangci-lint crit unable to find 'v1.37.0' - use 'latest' or see https://github.com/golangci/golangci-lint/releases for details
make[1]: *** [goget-tools] Error 1
make[1]: Leaving directory `/go/src/github.com/redhat-developer/odo'

/override ci/prow/unit

Unit tests passing in IBM Cloud.

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 merged commit b9507c9 into redhat-developer:main Jun 28, 2022
cdrage pushed a commit to cdrage/odo that referenced this pull request Aug 31, 2022
…edhat-developer#5878)

* Add documentation

* Add integration test cases

* Refactor functions in 'pkg/libdevfile/libdevfile.go'

Few functions used to have similar but slightly different logic,
which could make them quite confusing to use.
This also fixes a behavior which is not supposed to be
part of the Devfile spec: in the past, if a Devfile contains a single
but non-default command for a given kind, this command would get returned
as the Default command for such kind.
Now, an explicit error is returned instead if this single command
is not marked as default in the Devfile.

* Add `run-command` to `odo dev`, to run non-default run commands

* Fix documentation of a few libdevfile functions, as suggested in review
@rm3l rm3l deleted the 5775-odo-dev-add-flag-to-run-non-default-run-command branch December 1, 2022 16:35
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. area/dev Issues or PRs related to `odo dev` 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 dev add flag to run non-default run command.
4 participants