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

Refactor: fix s2i flag for odo create #4075

Merged
merged 3 commits into from
Oct 24, 2020
Merged

Refactor: fix s2i flag for odo create #4075

merged 3 commits into from
Oct 24, 2020

Conversation

devang-gaur
Copy link
Contributor

What type of PR is this?

/kind bug
/kind code-refactoring

Which issue(s) this PR fixes:
Fixes #4048 , #4018

How to test changes / Special notes to the reviewer:

Please try out the odo create command using all the possible combinations and flags.

@openshift-ci-robot openshift-ci-robot added kind/bug Categorizes issue or PR as related to a bug. kind/code-refactoring labels Oct 3, 2020
@codecov
Copy link

codecov bot commented Oct 3, 2020 •
edited
Loading

Codecov Report

Merging #4075 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4075   +/-   ##
=======================================
  Coverage   43.33%   43.33%           
=======================================
  Files         147      147           
  Lines       12456    12456           
=======================================
  Hits         5398     5398           
  Misses       6486     6486           
  Partials      572      572

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 49853dc...0bb7d35. Read the comment docs.

@devang-gaur
Copy link
Contributor Author

image

@prietyc123 satisfied with the message, when devfile does not exist?

cdrage
cdrage suggested changes Oct 6, 2020
View reviewed changes
Copy link
Member

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

Few changes, otherwise LGTM

pkg/odo/cli/component/create.go
if !co.forceS2i {
err = co.checkConflictingFlags()
if err != nil {
return
Copy link
Member

Choose a reason for hiding this comment

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

return... what? This should be returning err

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its defined here. https://github.com/openshift/odo/blob/ce72fb36681631b908ad7ca4731b2c4623db9823/pkg/odo/cli/component/create.go#L345

That's a golang feature you can define the variable in the return block of function definition

pkg/odo/cli/component/create.go Outdated
@@ -542,7 +539,7 @@ func (co *CreateOptions) Complete(name string, cmd *cobra.Command, args []string

if util.CheckPathExists(co.DevfilePath) || co.devfileMetadata.devfilePath.value != "" {
// Categorize the sections
log.Info("Validation")
log.Info("Validation for Devfile component")
Copy link
Member

Choose a reason for hiding this comment

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

Don't change this IMO, it should just stay as Validation. No need to specify Devfile. We're trying to avoid using the word Devfile anyways in the UI, since it's actually what's being used "under the hood" / engine, rather than what the user will be directly interacting with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @cdrage , If we keep it just as Validation then the logs come out as following if an S2I component is created by default.

image

IMO, some verbosity could actually help in this case. Also, this verbosity will actually fix #4018

@girishramnani @kadel @adisky @mik-dass you thoughts are welcome 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.

Currently it looks like this.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can revert back to just Validation after we get rid of S2I totally ?

Copy link
Contributor

@adisky adisky Oct 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think using only Validation is ok, explicitly specifying Validation for devfile component/ Validation for S2I component does not seem very user friendly. To solve the problem in better way we can use a common spinner Validating the component if one of the s2i or devfile component found just continue with it.

$ odo create java
Validation 
 ✓  Validating the component

if no s2i or devfile component found give a common error

$ odo create abcd
Validation 
 ✗ No component found, please run `odo catalog list components` for a list of supported components.

This way we could avoid the confusing output we have now

$ odo create mmmd
Validation
 ✗  Checking devfile existence [178883ns]
 ⚠  Devfile component type mmmd is not supported, please run `odo catalog list components` for a list of supported devfile component types
 ✗  component type "mmmd" not found

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @adisky we should keep it as Validation. We should not be outputting both s2i and devfile in the output either...

Having both would indeed confused the user

pkg/odo/cli/component/create.go
hasComponent := false
devfileExistSpinner := log.Spinner("Checking devfile existence")
hasComponent := false
var devfileExistSpinner *log.Status
Copy link
Member

Choose a reason for hiding this comment

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

As someone who wrote the log library and functionality, I'd suggest we avoid declaring a spinner like this..

You may run into issues in the future where the spinner will encounter a nil pointer.

pkg/odo/cli/component/create.go Outdated
@@ -849,7 +855,7 @@ func (co *CreateOptions) Validate() (err error) {
return nil
}

log.Info("Validation")
log.Info("Validation for S2I component")
Copy link
Member

Choose a reason for hiding this comment

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

Just keep it as Validation, I don't think we should declare that it's S2I.. As that would confuse the user.

@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 Oct 19, 2020
@girishramnani
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Oct 22, 2020
@openshift-bot
Copy link

/retest

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

6 similar comments
@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

3 similar comments
@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@kadel
Copy link
Member

kadel commented Oct 23, 2020

/retest

@openshift-bot
Copy link

/retest

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

4 similar comments
@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit ccfbd9b into redhat-developer:master Oct 24, 2020
@rm3l rm3l added the area/refactoring Issues or PRs related to code refactoring label Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnmcollier johnmcollier Awaiting requested review from johnmcollier

@prietyc123 prietyc123 Awaiting requested review from prietyc123

@girishramnani girishramnani Awaiting requested review from girishramnani

@adisky adisky Awaiting requested review from adisky

@cdrage cdrage Awaiting requested review from cdrage

Assignees

@cdrage cdrage

@girishramnani girishramnani

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. area/refactoring Issues or PRs related to code refactoring kind/bug Categorizes issue or PR as related to a bug. 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.

odo create: --s2i flag is broken
9 participants
@devang-gaur @girishramnani @openshift-ci-robot @openshift-bot @kadel @cdrage @adisky @rm3l @openshift-merge-robot