Preference cleanup (1/n) #5822

valaparthvi · 2022-06-14T05:47:04Z

What type of PR is this:
/kind cleanup

What does this PR do / why we need it:
This PR cleans the preference pkg with the following changes:

Use time.Duration instead of int for time related preferences.
Modify the success messages for set, and unset.

Which issue(s) this PR fixes:

Fixes part of #5535

PR acceptance criteria:

Unit test
Integration test
Documentation

How to test changes / Special notes to the reviewer:

odo preference set pushtimeout 4m
odo preference unset pushtimeout

openshift-ci · 2022-06-14T05:47:08Z

@valaparthvi: The label(s) kind/cleanup cannot be applied, because the repository doesn't have them.

In response to this:

What type of PR is this:
/kind cleanup

What does this PR do / why we need it:
This PR cleans the preference pkg with the following changes:

Use time.Duration instead of int for time related preferences.

Modify the success messages for set, and unset.

Which issue(s) this PR fixes:

Fixes part of #5535

PR acceptance criteria:

Unit test

Integration test

Documentation

How to test changes / Special notes to the reviewer:

odo preference set pushtimeout 4m

odo preference unset pushtimeout

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.

netlify · 2022-06-14T05:47:09Z

✅ Deploy Preview for odo-docusaurus-preview canceled.

Name	Link
🔨 Latest commit	`ddb6efc`
🔍 Latest deploy log	https://app.netlify.com/sites/odo-docusaurus-preview/deploys/62b423924bedc60008e932ea

odo-robot · 2022-06-14T05:53:55Z

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

odo-robot · 2022-06-14T06:00:23Z

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

odo-robot · 2022-06-14T06:00:34Z

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

odo-robot · 2022-06-14T06:02:07Z

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

odo-robot · 2022-06-14T06:02:33Z

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

pkg/odo/cli/preference/view_test.go

pkg/util/defaults.go

feloy · 2022-06-15T14:11:00Z

/lgtm

valaparthvi · 2022-06-15T14:33:08Z

/retest-required

valaparthvi · 2022-06-15T15:16:58Z

/test unit

valaparthvi · 2022-06-15T15:35:52Z

/retest-required

valaparthvi · 2022-06-16T09:53:59Z

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

IBM tests pass.

openshift-ci · 2022-06-16T09:54:08Z

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

In response to this:

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

IBM tests pass.

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 · 2022-06-17T07:23:09Z

When executing:

$ odo preference set PushTimeout 1
 ✗  unable to set "pushtimeout" to "1", value must be an integer

the message doesn't seem correct. It should be instead "value should be a duration"

feloy · 2022-06-17T07:44:48Z

I can see this pattern in several places, which gives the wrong result:

time.Duration(o.clientset.PreferenceClient.GetTimeout()) * time.Second

Is should be:

o.clientset.PreferenceClient.GetTimeout()

feloy · 2022-06-17T09:34:19Z

pkg/odo/cli/version/version.go

@@ -63,7 +64,7 @@ func (o *VersionOptions) Complete(cmdline cmdline.Cmdline, args []string) (err e
 			// checking the value of timeout in preference
 			var timeout time.Duration
 			if o.clientset.PreferenceClient != nil {
-				timeout = time.Duration(o.clientset.PreferenceClient.GetTimeout()) * time.Second
+				timeout = o.clientset.PreferenceClient.GetTimeout() * time.Second


I don't think you need to multiply by 1s (same in line 70)

You're right, I missed this one.

Signed-off-by: Parthvi Vala <[email protected]>

…e type

Signed-off-by: Parthvi Vala <[email protected]>

valaparthvi · 2022-06-20T12:36:55Z

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

openshift-ci · 2022-06-20T12:37:03Z

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

In response to this:

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

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 · 2022-06-21T14:44:35Z

pkg/preference/implem.go

-			}
-			if typedval < 0 {
-				return errors.New("cannot set timeout to less than 0")
+			typedval, err := time.ParseDuration(value)


If you want to convert the old value (in seconds) to the new one (in duration), you could test if the value is a pure integer here. If it is, you could consider it is an old value, and convert it in n* second duration.

Also, you could factorize the code for all the duration fields with a function

You're right, this is what you meant by the migration plan? Thanks for catching this!

Yes, that's about the migration plan I was talking about

rm3l · 2022-06-21T15:47:49Z

pkg/preference/implem.go

-				return errors.New("cannot set timeout to less than 0")
+			typedval, err := time.ParseDuration(value)
+			if err != nil || typedval < minimumDurationValue {
+				return fmt.Errorf("unable to set %q to %q, value must be a positive Duration (e.g. 4s, 5m, 1h); minimum value: 1s", parameter, value)


Wondering if the error we return here should not also wrap the original err returned by time.ParseDuration, if any. This way, it would be reported to the user. What do you think?

Because if I enter a duration but with a unit suffix (like days) not supported by time.ParseDuration, I find our error message not much helpful in understanding the issue:

❯ odo preference set PushTimeout 1d ✗ unable to set "pushtimeout" to "1d", value must be a positive Duration (e.g. 4s, 5m, 1h); minimum value: 1s

Wondering if the error we return here should not also wrap the original err returned by time.ParseDuration, if any. This way, it would be reported to the user. What do you think?

You're right, I'll get to this.

rm3l · 2022-06-21T15:55:43Z

pkg/preference/variables.go


 // PushTimeoutSettingDescription adds a description for PushTimeout
-var PushTimeoutSettingDescription = fmt.Sprintf("PushTimeout (in seconds) for waiting for a Pod to come up (Default: %d)", DefaultPushTimeout)
+var PushTimeoutSettingDescription = fmt.Sprintf("PushTimeout (in Duration) for waiting for a Pod to come up (Default: %s)", DefaultPushTimeout)


Surely nitpicking ^^, but would it be possible to change the format string? I found it a bit weird that this shows up as 4m0s in the help message. Same thing for the default values of the other durations, except for Timeout:

❯ odo help preference Modifies odo specific configuration settings within the global preference file. Available Global Parameters: ConsentTelemetry - If true, odo will collect telemetry for the user's odo usage (Default: false) For more information: https://developers.redhat.com/article/tool-data-collection Ephemeral - If true, odo will create an emptyDir volume to store source code (Default: true) PushTimeout - PushTimeout (in Duration) for waiting for a Pod to come up (Default: 4m0s) RegistryCacheTime - For how long (in Duration) odo will cache information from the Devfile registry (Default: 15m0s) Timeout - Timeout (in Duration) for cluster server connection check (Default: 1s) UpdateNotification - Flag to control if an update notification is shown or not (Default: true)

That format is how golang parses duration. I'll have to see if that can be modified, but another way would be to parse everything into second.

valaparthvi · 2022-06-23T08:17:16Z

ddb6efc includes

@feloy - a warning as a migration plan from v2 to v3, see the integration test on how to reproduce it; it still takes the value as a ns, and prints out warning a bunch of times since newPreferenceInfo is called at least 4 times, I understand that it is annoying and not a good user experience, but I think the more annoyed you are, the more likely you are to make the change. I also did not implement the part about checking if the value is an int while setting preference, because I think it's counterintuitive to the first part of this statement. If we do not want to parse the existing int value, it also makes sense to not accept it in the first place even if it eventually converted to a time.Second.
@rm3l - a better error message when setting the preference value fails due to an incompatible format or any other cause

…patible formats Signed-off-by: Parthvi Vala <[email protected]>

sonarqubecloud · 2022-06-23T08:26:46Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

No Coverage information
0.4% Duplication

feloy · 2022-06-23T10:20:37Z

/approve

openshift-ci · 2022-06-23T10:20:45Z

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

~~OWNERS~~ [feloy]

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

valaparthvi · 2022-06-23T11:32:42Z

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

openshift-ci · 2022-06-23T11:32:51Z

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

In response to this:

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

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

Thanks for the changes!

feloy · 2022-06-23T14:26:07Z

/override ci/prow/unit
Pass on IBM

openshift-ci · 2022-06-23T14:26:11Z

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

In response to this:

/override ci/prow/unit
Pass on IBM

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.

* Preference(Timeout) int > time.Duration * Fix odo preference --help for unset examples * Preference(PushTimeout) int > time.Duration * Change set, and unset messages * Preference(RegistryCacheTime) int > time.Duration * use cobra.ExactArgs for set, and unset * mockgen * Unit tests and integration tests * Fix unit test failure * Update k8s.io/utils pkg * Philippe's review * Fix error message * Philippe's review Signed-off-by: Parthvi Vala <[email protected]> * Add minimum acceptable value for preferences accepting time.Difference type * Fix unit test failure Signed-off-by: Parthvi Vala <[email protected]> * Add migration plan with a warning, add better error message for incompatible formats Signed-off-by: Parthvi Vala <[email protected]>

openshift-ci bot requested review from anandrkskd and rm3l June 14, 2022 05:47

valaparthvi mentioned this pull request Jun 14, 2022

odo preference - cleanup #5535

Closed

7 tasks

feloy reviewed Jun 14, 2022

View reviewed changes

pkg/odo/cli/preference/view_test.go Outdated Show resolved Hide resolved

pkg/util/defaults.go Outdated Show resolved Hide resolved

valaparthvi requested review from kadel and dharmit June 14, 2022 13:22

openshift-ci bot assigned feloy Jun 15, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 15, 2022

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 17, 2022

feloy suggested changes Jun 17, 2022

View reviewed changes

valaparthvi force-pushed the preference-cleanup branch 2 times, most recently from 397e472 to d21547f Compare June 17, 2022 11:03

feloy closed this Jun 17, 2022

feloy reopened this Jun 17, 2022

valaparthvi force-pushed the preference-cleanup branch from d21547f to 36cc352 Compare June 17, 2022 12:42

valaparthvi added 6 commits June 20, 2022 16:26

Update k8s.io/utils pkg

0961c1e

Philippe's review

961bae0

Fix error message

c60b281

Philippe's review

48b0741

Signed-off-by: Parthvi Vala <[email protected]>

Add minimum acceptable value for preferences accepting time.Differenc…

91c83ef

…e type

Fix unit test failure

51cae65

Signed-off-by: Parthvi Vala <[email protected]>

valaparthvi force-pushed the preference-cleanup branch from 5335bf0 to 51cae65 Compare June 20, 2022 10:57

valaparthvi requested review from rm3l and feloy June 21, 2022 06:20

feloy suggested changes Jun 21, 2022

View reviewed changes

rm3l reviewed Jun 21, 2022

View reviewed changes

Add migration plan with a warning, add better error message for incom…

ddb6efc

…patible formats Signed-off-by: Parthvi Vala <[email protected]>

valaparthvi force-pushed the preference-cleanup branch from 3f53cdf to ddb6efc Compare June 23, 2022 08:25

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 23, 2022

valaparthvi requested a review from rm3l June 23, 2022 11:33

rm3l approved these changes Jun 23, 2022

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 23, 2022

openshift-ci bot merged commit b1fbfaa into redhat-developer:main Jun 23, 2022

dharmit mentioned this pull request Sep 9, 2022

Guide for migrating from v2 to v3 #6095

Merged

3 tasks

Preference cleanup (1/n) #5822

Preference cleanup (1/n) #5822

Conversation

valaparthvi commented Jun 14, 2022

openshift-ci bot commented Jun 14, 2022

netlify bot commented Jun 14, 2022 • edited Loading

✅ Deploy Preview for odo-docusaurus-preview canceled.

odo-robot bot commented Jun 14, 2022 • edited Loading

odo-robot bot commented Jun 14, 2022 • edited Loading

odo-robot bot commented Jun 14, 2022 • edited Loading

odo-robot bot commented Jun 14, 2022 • edited Loading

odo-robot bot commented Jun 14, 2022 • edited Loading

feloy commented Jun 15, 2022

valaparthvi commented Jun 15, 2022

valaparthvi commented Jun 15, 2022

valaparthvi commented Jun 15, 2022

valaparthvi commented Jun 16, 2022

openshift-ci bot commented Jun 16, 2022

feloy commented Jun 17, 2022

feloy commented Jun 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valaparthvi commented Jun 20, 2022

openshift-ci bot commented Jun 20, 2022

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

rm3l Jun 21, 2022 • edited Loading

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

valaparthvi commented Jun 23, 2022 • edited Loading

sonarqubecloud bot commented Jun 23, 2022

feloy commented Jun 23, 2022

openshift-ci bot commented Jun 23, 2022

valaparthvi commented Jun 23, 2022

openshift-ci bot commented Jun 23, 2022

rm3l left a comment

Choose a reason for hiding this comment

feloy commented Jun 23, 2022

openshift-ci bot commented Jun 23, 2022

netlify bot commented Jun 14, 2022 •

edited

Loading

odo-robot bot commented Jun 14, 2022 •

edited

Loading

odo-robot bot commented Jun 14, 2022 •

edited

Loading

odo-robot bot commented Jun 14, 2022 •

edited

Loading

odo-robot bot commented Jun 14, 2022 •

edited

Loading

odo-robot bot commented Jun 14, 2022 •

edited

Loading

rm3l Jun 21, 2022 •

edited

Loading

valaparthvi commented Jun 23, 2022 •

edited

Loading