Fix preference not taken into account for `odo version` #6415

feloy · 2022-12-15T09:57:40Z

What type of PR is this:

/kind bug

What does this PR do / why we need it:

This PR adds the PREFERNECE dependency to odo version, and creates a CommandGroup abstraction to avoid this error occurs again.

Which issue(s) this PR fixes:

odo version can use the Timeout value defined in the odo preferences, but this value were never taken into account, because the PREFERENCE dependency were overwritten when setting the command group.

Fixes #6417

PR acceptance criteria:

Unit test
Integration test
Documentation

How to test changes / Special notes to the reviewer:

netlify · 2022-12-15T09:57:45Z

✅ Deploy Preview for odo-docusaurus-preview ready!

Name	Link
🔨 Latest commit	`c934f19`
🔍 Latest deploy log	https://app.netlify.com/sites/odo-docusaurus-preview/deploys/639b154836789000071e99bf
😎 Deploy Preview	https://deploy-preview-6415--odo-docusaurus-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

odo-robot · 2022-12-15T10:03:49Z

OpenShift Tests on commit e96f123 finished with errors.
View logs: TXT HTML

odo-robot · 2022-12-15T10:03:55Z

NoCluster Tests on commit e96f123 finished successfully.
View logs: TXT HTML

odo-robot · 2022-12-15T10:05:17Z

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

odo-robot · 2022-12-15T10:22:57Z

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

odo-robot · 2022-12-15T10:27:15Z

Windows Tests (OCP) on commit e96f123 finished with errors.
View logs: TXT HTML

odo-robot · 2022-12-15T10:28:41Z

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

rm3l

Good catch - thanks!

I'm wondering in this case if we shouldn't make the preference client a mandatory dependency, and explicitly fail if this is not the case.
I can see that preferenceInfo#GetTimeout() already falls back to DefaultTimeout if there is no timeout setting defined. So we could be more strict by expecting the Preference client dependency to be there in VersionOptions#Complete.
To be clear, I am just thinking of the following diff:

diff --git a/pkg/odo/cli/version/version.go b/pkg/odo/cli/version/version.go
index 180c8b58a..e31226808 100644
--- a/pkg/odo/cli/version/version.go
+++ b/pkg/odo/cli/version/version.go
@@ -5,13 +5,11 @@ import (
        "fmt"
        "os"
        "strings"
-       "time"
 
        "github.com/redhat-developer/odo/pkg/kclient"
        "github.com/redhat-developer/odo/pkg/odo/cmdline"
        "github.com/redhat-developer/odo/pkg/odo/genericclioptions"
        "github.com/redhat-developer/odo/pkg/odo/genericclioptions/clientset"
-       "github.com/redhat-developer/odo/pkg/preference"
        odoversion "github.com/redhat-developer/odo/pkg/version"
 
        "github.com/spf13/cobra"
@@ -63,16 +61,7 @@ func (o *VersionOptions) Complete(ctx context.Context, cmdline cmdline.Cmdline,
                client, err := kclient.New()
 
                if err == nil {
-                       // checking the value of timeout in preference
-                       var timeout time.Duration
-                       if o.clientset.PreferenceClient != nil {
-                               timeout = o.clientset.PreferenceClient.GetTimeout()
-                       } else {
-                               // the default timeout will be used
-                               // when the value is not readable from preference
-                               timeout = preference.DefaultTimeout
-                       }
-                       o.serverInfo, err = client.GetServerVersion(timeout)
+                       o.serverInfo, err = client.GetServerVersion(o.clientset.PreferenceClient.GetTimeout())
                        if err != nil {
                                klog.V(4).Info("unable to fetch the server version: ", err)
                        }

At least, this would explicitly panic if the Preference Client is not there and allow to catch this issue if it occurs again, IMO.

What do you think?

feloy · 2022-12-15T12:31:27Z

Good catch - thanks!

I'm wondering in this case if we shouldn't make the preference client a mandatory dependency, and explicitly fail if this is not the case. I can see that preferenceInfo#GetTimeout() already falls back to DefaultTimeout if there is no timeout setting defined. So we could be more strict by expecting the Preference client dependency to be there in VersionOptions#Complete. To be clear, I am just thinking of the following diff:

diff --git a/pkg/odo/cli/version/version.go b/pkg/odo/cli/version/version.go
index 180c8b58a..e31226808 100644
--- a/pkg/odo/cli/version/version.go
+++ b/pkg/odo/cli/version/version.go
@@ -5,13 +5,11 @@ import (
        "fmt"
        "os"
        "strings"
-       "time"
 
        "github.com/redhat-developer/odo/pkg/kclient"
        "github.com/redhat-developer/odo/pkg/odo/cmdline"
        "github.com/redhat-developer/odo/pkg/odo/genericclioptions"
        "github.com/redhat-developer/odo/pkg/odo/genericclioptions/clientset"
-       "github.com/redhat-developer/odo/pkg/preference"
        odoversion "github.com/redhat-developer/odo/pkg/version"
 
        "github.com/spf13/cobra"
@@ -63,16 +61,7 @@ func (o *VersionOptions) Complete(ctx context.Context, cmdline cmdline.Cmdline,
                client, err := kclient.New()
 
                if err == nil {
-                       // checking the value of timeout in preference
-                       var timeout time.Duration
-                       if o.clientset.PreferenceClient != nil {
-                               timeout = o.clientset.PreferenceClient.GetTimeout()
-                       } else {
-                               // the default timeout will be used
-                               // when the value is not readable from preference
-                               timeout = preference.DefaultTimeout
-                       }
-                       o.serverInfo, err = client.GetServerVersion(timeout)
+                       o.serverInfo, err = client.GetServerVersion(o.clientset.PreferenceClient.GetTimeout())
                        if err != nil {
                                klog.V(4).Info("unable to fetch the server version: ", err)
                        }

At least, this would explicitly panic if the Preference Client is not there and allow to catch this issue if it occurs again, IMO.

What do you think?

You are right, I'm also annoyed by this test on the client, which was hiding the error.

I'll apply your patch.
Thanks

Co-authored-by: Armel Soro <[email protected]>

sonarqubecloud · 2022-12-15T12:39:37Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

rm3l · 2022-12-15T12:58:03Z

cmd/odo/help_test.go

+`
+)
+
+func TestOdoHelp(t *testing.T) {


Thanks for this test 😍 💯

feloy · 2022-12-15T14:02:54Z

/override windows-integration-test/Windows-test
Flaky e2e test

 [FAILED] [208.773 seconds]
E2E Test
C:/Users/Administrator.ANSIBLE-TEST-VS/2778/tests/e2escenarios/e2e_test.go:19
  starting with non-empty Directory add Binding
  C:/Users/Administrator.ANSIBLE-TEST-VS/2778/tests/e2escenarios/e2e_test.go:279
    [It] should verify developer workflow of using binding as env in innerloop
    C:/Users/Administrator.ANSIBLE-TEST-VS/2778/tests/e2escenarios/e2e_test.go:321

openshift-ci · 2022-12-15T14:02:58Z

@feloy: Overrode contexts on behalf of feloy: windows-integration-test/Windows-test

In response to this:

/override windows-integration-test/Windows-test
Flaky e2e test

[FAILED] [208.773 seconds]
E2E Test
C:/Users/Administrator.ANSIBLE-TEST-VS/2778/tests/e2escenarios/e2e_test.go:19
 starting with non-empty Directory add Binding
 C:/Users/Administrator.ANSIBLE-TEST-VS/2778/tests/e2escenarios/e2e_test.go:279
   [It] should verify developer workflow of using binding as env in innerloop
   C:/Users/Administrator.ANSIBLE-TEST-VS/2778/tests/e2escenarios/e2e_test.go:321

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-12-15T14:04:43Z

/override OpenShift-Integration-tests/OpenShift-Integration-tests
Unrelated flaky test

[FAILED] [238.740 seconds]
odo dev command tests
/go/odo_1/tests/integration/cmd_dev_test.go:31
  port-forwarding for the component
  /go/odo_1/tests/integration/cmd_dev_test.go:595
    when devfile has single endpoint
    /go/odo_1/tests/integration/cmd_dev_test.go:596
      when running odo dev
      /go/odo_1/tests/integration/cmd_dev_test.go:605
        when modifying memoryLimit for container in Devfile [BeforeEach]
        /go/odo_1/tests/integration/cmd_dev_test.go:639
          should expose the endpoint on localhost
          /go/odo_1/tests/integration/cmd_dev_test.go:655

openshift-ci · 2022-12-15T14:04:47Z

@feloy: Overrode contexts on behalf of feloy: OpenShift-Integration-tests/OpenShift-Integration-tests

In response to this:

/override OpenShift-Integration-tests/OpenShift-Integration-tests
Unrelated flaky test

[FAILED] [238.740 seconds]
odo dev command tests
/go/odo_1/tests/integration/cmd_dev_test.go:31
 port-forwarding for the component
 /go/odo_1/tests/integration/cmd_dev_test.go:595
   when devfile has single endpoint
   /go/odo_1/tests/integration/cmd_dev_test.go:596
     when running odo dev
     /go/odo_1/tests/integration/cmd_dev_test.go:605
       when modifying memoryLimit for container in Devfile [BeforeEach]
       /go/odo_1/tests/integration/cmd_dev_test.go:639
         should expose the endpoint on localhost
         /go/odo_1/tests/integration/cmd_dev_test.go:655

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 added 3 commits December 14, 2022 15:20

Quick fix

d5d97c4

Test grouping of comands in odo help

87ddbee

Create a CommandGroup abstraction

df0eb32

openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Dec 15, 2022

openshift-ci bot requested review from kadel and valaparthvi December 15, 2022 09:57

rm3l reviewed Dec 15, 2022

View reviewed changes

Expect a non-nil Preference Client

c934f19

Co-authored-by: Armel Soro <[email protected]>

rm3l approved these changes Dec 15, 2022

View reviewed changes

cmd/odo/help_test.go

`

)

func TestOdoHelp(t *testing.T) {

Copy link

Member

rm3l Dec 15, 2022 •

edited

Loading

Choose a reason for hiding this comment

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

Thanks for this test 😍 💯

openshift-ci bot assigned rm3l Dec 15, 2022

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

openshift-merge-robot merged commit 1807939 into redhat-developer:main Dec 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix preference not taken into account for `odo version` #6415

Fix preference not taken into account for `odo version` #6415

feloy commented Dec 15, 2022 •

edited

Loading

netlify bot commented Dec 15, 2022 •

edited

Loading

odo-robot bot commented Dec 15, 2022 •

edited

Loading

odo-robot bot commented Dec 15, 2022 •

edited

Loading

odo-robot bot commented Dec 15, 2022 •

edited

Loading

odo-robot bot commented Dec 15, 2022 •

edited

Loading

odo-robot bot commented Dec 15, 2022 •

edited

Loading

odo-robot bot commented Dec 15, 2022 •

edited

Loading

rm3l left a comment

feloy commented Dec 15, 2022

sonarqubecloud bot commented Dec 15, 2022

rm3l Dec 15, 2022 •

edited

Loading

feloy commented Dec 15, 2022

openshift-ci bot commented Dec 15, 2022

feloy commented Dec 15, 2022

openshift-ci bot commented Dec 15, 2022

Fix preference not taken into account for odo version #6415

Fix preference not taken into account for odo version #6415

Conversation

feloy commented Dec 15, 2022 • edited Loading

netlify bot commented Dec 15, 2022 • edited Loading

✅ Deploy Preview for odo-docusaurus-preview ready!

odo-robot bot commented Dec 15, 2022 • edited Loading

odo-robot bot commented Dec 15, 2022 • edited Loading

odo-robot bot commented Dec 15, 2022 • edited Loading

odo-robot bot commented Dec 15, 2022 • edited Loading

odo-robot bot commented Dec 15, 2022 • edited Loading

odo-robot bot commented Dec 15, 2022 • edited Loading

rm3l left a comment

Choose a reason for hiding this comment

feloy commented Dec 15, 2022

sonarqubecloud bot commented Dec 15, 2022

rm3l Dec 15, 2022 • edited Loading

Choose a reason for hiding this comment

feloy commented Dec 15, 2022

openshift-ci bot commented Dec 15, 2022

feloy commented Dec 15, 2022

openshift-ci bot commented Dec 15, 2022

Fix preference not taken into account for `odo version` #6415

Fix preference not taken into account for `odo version` #6415

feloy commented Dec 15, 2022 •

edited

Loading

netlify bot commented Dec 15, 2022 •

edited

Loading

odo-robot bot commented Dec 15, 2022 •

edited

Loading

odo-robot bot commented Dec 15, 2022 •

edited

Loading

odo-robot bot commented Dec 15, 2022 •

edited

Loading

odo-robot bot commented Dec 15, 2022 •

edited

Loading

odo-robot bot commented Dec 15, 2022 •

edited

Loading

odo-robot bot commented Dec 15, 2022 •

edited

Loading

rm3l Dec 15, 2022 •

edited

Loading