Adds disclaimer that experimental mode is enabled #2786

cdrage · 2020-03-31T14:23:18Z

What type of PR is this?

/kind documentation

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

Adds a disclaimer that experimental mode is enabled when using odo create or odo push.

See asciinema:

Which issue(s) this PR fixes:

N/A

How to test changes / Special notes to the reviewer:

odo preference set experimental true
git clone https://github.com/openshift/nodejs-ex
odo create nodejs
odo push

Signed-off-by: Charlie Drage charlie@charliedrage.com

codecov · 2020-03-31T14:37:13Z

Codecov Report

Merging #2786 into master will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2786      +/-   ##
==========================================
+ Coverage   43.60%   43.64%   +0.04%     
==========================================
  Files          94       94              
  Lines        8726     8743      +17     
==========================================
+ Hits         3805     3816      +11     
- Misses       4557     4563       +6     
  Partials      364      364

Impacted Files	Coverage Δ
...g/devfile/adapters/kubernetes/component/adapter.go	`26.94% <0.00%> (+1.43%)`	⬆️
pkg/kclient/pods.go	`43.83% <0.00%> (+2.40%)`	⬆️

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 5931049...b5e4cb0. Read the comment docs.

cdrage · 2020-03-31T17:24:50Z

/retest

amitkrout · 2020-03-31T19:16:14Z

Platform issue
/retest

mik-dass · 2020-04-01T11:23:50Z

tests/integration/devfile/cmd_devfile_push_test.go

+			experimentalOutputMsg := "Experimental mode is enabled, use at your own risk"
+			Expect(helper.CmdShouldPass("odo", "create", "nodejs")).To(ContainSubstring(experimentalOutputMsg))
+			Expect(helper.CmdShouldPass("odo", "push")).To(ContainSubstring(experimentalOutputMsg))
+


also a check that when experimental is turned off, this string is gone

Ah yes, I'll add that.

mik-dass · 2020-04-01T11:29:14Z

pkg/log/status.go

@@ -197,6 +197,14 @@ func Swarningf(format string, a ...interface{}) string {
 	return fmt.Sprintf(" %s%s%s", yellow(getWarningString()), suffixSpacing, fmt.Sprintf(format, a...))
 }

+// Warning will output in an appropriate "progress" manner
+func Experimental(a ...interface{}) {


can't we just use some of the other functions for this? like the warning function below

Ah no, because that'll add the whole warning symbol: ⚠️ on it. Which I believe we shouldn't need / it looks ugly.

TBH I think the warning symbol would look appropriate

The reason I made this function separate is so we do not have the indenting and the warning symbol, for example:

Experimental mode is enabled, use at your own risk Validation ✓ Checking Devfile compatibility [43501ns] ✓ Validating Devfile [38035ns]

vs

⚠️ Experimental mode is enabled, use at your own risk Validation ✓ Checking Devfile compatibility [43501ns] ✓ Validating Devfile [38035ns]

IMO the bottom one looks kind of odd without the newline. So I went with the first one (separate function).

Yeah the newline makes more sense, but can we still keep the warning symbol. Looks cool and approriate TBH.

mohammedzee1000 · 2020-04-01T11:29:31Z

@cdrage rebase please and please address mrinal's comments. Otherwise looks good to me

mik-dass · 2020-04-01T11:30:28Z

pkg/log/status.go

@@ -197,6 +197,14 @@ func Swarningf(format string, a ...interface{}) string {
 	return fmt.Sprintf(" %s%s%s", yellow(getWarningString()), suffixSpacing, fmt.Sprintf(format, a...))
 }

+// Warning will output in an appropriate "progress" manner


warning->Experimental

Woops. Thanks!

cdrage · 2020-04-01T13:16:46Z

@mik-dass your comments have been addressed 👍

mik-dass · 2020-04-01T13:42:39Z

@mik-dass your comments have been addressed +1

you forgot https://github.com/openshift/odo/pull/2786/files#r401546048 :D

cdrage · 2020-04-03T18:27:52Z

/retest

amitkrout · 2020-04-05T12:47:37Z

tests/integration/devfile/cmd_devfile_push_test.go

+			helper.CopyExample(filepath.Join("source", "nodejs"), context)
+
+			// Check that it will contain the experimental mode output
+			experimentalOutputMsg := "Experimental mode is enabled, use at your own risk"


It seems like we are exposing experimental feature putting original feature at risk. AFAIK experimental feature is some functionality/feature added to the original product to get more feedback from the user side and ofcoures there is a trade-off if it breaks but it should never have bad impact on the original product functionality/feature.

The message Experimental mode is enabled, use at your own risk exactly opposite what i mentioned above. Please counter my arguments

Ping @cdrage @mik-dass @girishramnani

@amitkrout This message just informs the user that experimental mode is enabled and somethings may work differently. So if a user activates this mode accidentally, he gets a warning and thus can choose to stop the process.

Cool!!!
@mik-dass Actually recently i came across a issue #2813, so thought the message is for original feature too. You can find certain scenario where your justification contradicts. For example

$ ODO_EXPERIMENTAL=true odo create nodejs --project test321 --context ../tests/examples/source/nodejs/

I am creating a regular nodejs component with experimental enabled.

You can find certain scenario where your justification contradicts.

Within the code itself, something might work differently for experimental mode. We are just informing the user that something might go wrong if the mode is activated, not that it will. It might pass without failure too.

For example odo push might push using the devfile if enabled or will push a normal nodejs component if disabled.

The warning will inform him straight away that push might work differently since he is in experimental mode and certain features might be missing.

johnmcollier · 2020-04-07T15:32:08Z

pkg/odo/cli/component/create.go

 	if experimental.IsExperimentalModeEnabled() {
+
+		// Add a disclaimer that we are in *experimental mode*
+		log.Experimental("Experimental mode is enabled, use at your own risk")


I agree with the general idea, but I'm not sure if I'd want to show the message for every odo create and odo push with experimental mode enabled.

I'm not sure how feasible this would be (you tell me 😉 ), but what about only showing it for the first (or first n) odo commands used by the user once it's enabled? Or maybe only showing it when odo preference set experimental true is called?

Ah, I don't think that'd be feasible for the first n commands.

But in terms of the output, I think it's viable since honestly, with me switching from set experimental true from false to true all the time testing PR's, I sometimes end up pushing a devfile instead of what I think is a regular component, since there was no output from odo create.

Perhaps we can output it only for odo create and that's it? What do you think?

Yeah, having it display during odo create seems reasonable to me 👍

AWesome! Updated.

**What type of PR is this?** /kind documentation **What does does this PR do / why we need it**: Adds a disclaimer that experimental mode is enabled when using `odo create` or `odo push`. See asciinema: [![asciicast](https://asciinema.org/a/A6ITI1wNQL9hahhLsCz4DT6wQ.svg)](https://asciinema.org/a/A6ITI1wNQL9hahhLsCz4DT6wQ) **Which issue(s) this PR fixes**: N/A **How to test changes / Special notes to the reviewer**: ```sh odo preference set experimental true git clone https://github.com/openshift/nodejs-ex odo create nodejs odo push ``` Signed-off-by: Charlie Drage <charlie@charliedrage.com>

girishramnani · 2020-04-08T03:15:08Z

/approve

openshift-ci-robot · 2020-04-08T03:15:57Z

[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:

~~OWNERS~~ [girishramnani]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

johnmcollier

Changes look good and work as expected for me. Having the message display during just odo create works nicely 👍

/lgtm

openshift-ci-robot added the kind/documentation label Mar 31, 2020

openshift-ci-robot requested review from maysunfaisal and mohammedzee1000 March 31, 2020 14:25

cdrage force-pushed the more-clear-about-experimental-mode branch 2 times, most recently from 1a3e2eb to 06c9d99 Compare March 31, 2020 14:31

mik-dass reviewed Apr 1, 2020

View reviewed changes

mohammedzee1000 added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Apr 1, 2020

mik-dass reviewed Apr 1, 2020

View reviewed changes

cdrage force-pushed the more-clear-about-experimental-mode branch from 06c9d99 to f71ec12 Compare April 1, 2020 13:16

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Apr 1, 2020

cdrage force-pushed the more-clear-about-experimental-mode branch from f71ec12 to f78a7e4 Compare April 1, 2020 19:14

amitkrout reviewed Apr 5, 2020

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Apr 5, 2020

cdrage force-pushed the more-clear-about-experimental-mode branch from f78a7e4 to 04f5637 Compare April 6, 2020 14:15

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Apr 6, 2020

cdrage force-pushed the more-clear-about-experimental-mode branch from 04f5637 to 4775465 Compare April 6, 2020 17:54

johnmcollier reviewed Apr 7, 2020

View reviewed changes

cdrage force-pushed the more-clear-about-experimental-mode branch from 4775465 to b5e4cb0 Compare April 7, 2020 19:18

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Apr 8, 2020

johnmcollier approved these changes Apr 8, 2020

View reviewed changes

openshift-ci-robot assigned johnmcollier Apr 8, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 8, 2020

openshift-merge-robot merged commit f47c9c9 into redhat-developer:master Apr 8, 2020

cdrage deleted the more-clear-about-experimental-mode branch January 14, 2022 14:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds disclaimer that experimental mode is enabled #2786

Adds disclaimer that experimental mode is enabled #2786

cdrage commented Mar 31, 2020

codecov bot commented Mar 31, 2020 •

edited

cdrage commented Mar 31, 2020

amitkrout commented Mar 31, 2020

mik-dass Apr 1, 2020

cdrage Apr 1, 2020

mik-dass Apr 1, 2020 •

edited

cdrage Apr 1, 2020

mik-dass Apr 1, 2020

cdrage Apr 1, 2020

mik-dass Apr 2, 2020

mohammedzee1000 commented Apr 1, 2020

mik-dass Apr 1, 2020

cdrage Apr 1, 2020

cdrage commented Apr 1, 2020

mik-dass commented Apr 1, 2020

cdrage commented Apr 3, 2020

amitkrout Apr 5, 2020 •

edited

mik-dass Apr 6, 2020

amitkrout Apr 6, 2020

mik-dass Apr 6, 2020 •

edited

johnmcollier Apr 7, 2020 •

edited

cdrage Apr 7, 2020

johnmcollier Apr 7, 2020

cdrage Apr 7, 2020

girishramnani commented Apr 8, 2020

openshift-ci-robot commented Apr 8, 2020

johnmcollier left a comment

Adds disclaimer that experimental mode is enabled #2786

Adds disclaimer that experimental mode is enabled #2786

Conversation

cdrage commented Mar 31, 2020

codecov bot commented Mar 31, 2020 • edited

Codecov Report

cdrage commented Mar 31, 2020

amitkrout commented Mar 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik-dass Apr 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohammedzee1000 commented Apr 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrage commented Apr 1, 2020

mik-dass commented Apr 1, 2020

cdrage commented Apr 3, 2020

amitkrout Apr 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik-dass Apr 6, 2020 • edited

Choose a reason for hiding this comment

johnmcollier Apr 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girishramnani commented Apr 8, 2020

openshift-ci-robot commented Apr 8, 2020

johnmcollier left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 31, 2020 •

edited

mik-dass Apr 1, 2020 •

edited

amitkrout Apr 5, 2020 •

edited

mik-dass Apr 6, 2020 •

edited

johnmcollier Apr 7, 2020 •

edited