Separate layers : Preference #5296

feloy · 2021-12-16T09:13:42Z

What type of PR is this?

/kind cleanup

What does this PR do / why we need it:

Which issue(s) this PR fixes:

Part of #5247

PR acceptance criteria:

Unit test
Integration test
Documentation
I have read the test guidelines

How to test changes / Special notes to the reviewer:

netlify · 2021-12-16T09:13:47Z

✔️ Deploy Preview for odo-docusaurus-preview canceled.

🔨 Explore the source changes: 2bfa3da

🔍 Inspect the deploy log: https://app.netlify.com/sites/odo-docusaurus-preview/deploys/61dc0a4f1b8bd30007f4768a

feloy · 2021-12-16T09:30:29Z

Unit Tests finished with errors.
View logs: TXT HTML

feloy · 2021-12-16T09:30:52Z

Kubernetes Tests finished successfully.
View logs: TXT HTML

feloy · 2021-12-16T09:38:06Z

OpenShift Tests finished successfully.
View logs: TXT HTML

feloy · 2021-12-16T09:48:41Z

Kubernetes Tests finished successfully.
View logs: TXT HTML

feloy · 2021-12-16T09:52:54Z

Unit Tests finished successfully.
View logs: TXT HTML

feloy · 2021-12-16T10:02:34Z

OpenShift Tests finished successfully.
View logs: TXT HTML

feloy · 2021-12-16T18:00:35Z

Kubernetes Tests finished successfully.
View logs: TXT HTML

feloy · 2021-12-16T18:01:23Z

Unit Tests finished with errors.
View logs: TXT HTML

feloy · 2021-12-16T18:07:58Z

OpenShift Tests finished successfully.
View logs: TXT HTML

odo-robot · 2021-12-17T14:25:03Z

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

pkg/preference/implem.go

valaparthvi · 2021-12-17T11:21:01Z

pkg/preference/implem.go

+// preferenceInfo wraps the preference and provides helpers to
+// serialize it.
+type preferenceInfo struct {
+	Filename   string `yaml:"FileName,omitempty"`
+	Preference `yaml:",omitempty"`
+}


I think it makes more sense to define this on line 174, after defining Preference type.

pkg/preference/implem.go

pkg/odo/cli/preference/unset_test.go

pkg/preference/implem.go

feloy · 2021-12-20T13:31:18Z

Unit Tests finished with errors.
View logs: TXT HTML

odo-robot · 2021-12-20T13:35:23Z

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

feloy · 2021-12-20T13:47:35Z

OpenShift Tests finished with errors.
View logs: TXT HTML

feloy · 2021-12-20T13:48:43Z

Kubernetes Tests finished successfully.
View logs: TXT HTML

feloy · 2022-01-06T08:42:45Z

/test v4.9-integration-e2e

  * could not run steps: step [release:latest] failed: release "release-latest" failed: the pod ci-op-1wjwi82z/release-latest was deleted without completing after 29s (failed containers: )

feloy · 2022-01-06T08:44:44Z

/test unit

  * could not run steps: step unit failed: "unit" test steps failed: "unit" pod "unit-unit-steps" failed: the pod ci-op-7nl9cs8b/unit-unit-steps was deleted without completing after 7m20s (failed containers: )

valaparthvi · 2022-01-10T05:59:12Z

pkg/kclient/pods_test.go

@@ -80,7 +80,7 @@ func TestWaitAndGetPodWithEvents(t *testing.T) {

 			podSelector := fmt.Sprintf("deploymentconfig=%s", tt.podName)

-			pod, err := fakeClient.WaitAndGetPodWithEvents(podSelector, corev1.PodRunning, "Waiting for component to start")
+			pod, err := fakeClient.WaitAndGetPodWithEvents(podSelector, corev1.PodRunning, "Waiting for component to start", time.Second)


Shouldn't this test with DefaultPushTimeout?

valaparthvi · 2022-01-10T06:00:34Z

pkg/odo/cli/component/create.go

 	return &CreateOptions{
-		PushOptions: NewPushOptions(prjClient),
+		PushOptions: NewPushOptions(prjClient, prefClient),
+		prefClient:  prefClient,


I don't think you will need a separate prefClient here. CreateOptions 'inherits' PushOptions, thereby getting access to prefClient.

valaparthvi · 2022-01-10T06:04:14Z

pkg/odo/cli/preference/view_test.go

+	prefClient.EXPECT().UpdateNotification()
+	prefClient.EXPECT().NamePrefix()
+	prefClient.EXPECT().Timeout()
+	prefClient.EXPECT().BuildTimeout()
+	prefClient.EXPECT().PushTimeout()
+	prefClient.EXPECT().EphemeralSourceVolume()
+	prefClient.EXPECT().ConsentTelemetry()


Don't you need to return something here?

Co-authored-by: Parthvi Vala <pvala@redhat.com>

valaparthvi

I have one more suggestion, but seems like things are working without it as well.

valaparthvi · 2022-01-10T12:14:08Z

pkg/odo/cli/preference/unset_test.go

+			}
+
+			if tt.exists || tt.forceFlag {
+				prefClient.EXPECT().DeleteConfiguration("arg1")


Should this return any error, or if nothing is mentioned, it returns a default value?

I cannot find any documentation on this, but I assume it will return the zero values for all the returned values if we don't specify any other value.

valaparthvi · 2022-01-10T13:04:54Z

/approve

openshift-ci · 2022-01-10T13:05:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: valaparthvi

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~~ [valaparthvi]

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

feloy · 2022-01-10T13:31:50Z

/test v4.9-integration-e2e

dial tcp 3.231.171.100:6443: connect: connection refused

openshift-ci · 2022-01-10T14:29:20Z

@feloy: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/psi-unit-test-mac	`2bfa3da`	link	false	`/test psi-unit-test-mac`
ci/prow/psi-unit-test-windows	`2bfa3da`	link	false	`/test psi-unit-test-windows`
ci/prow/psi-kubernetes-integration-e2e	`2bfa3da`	link	false	`/test psi-kubernetes-integration-e2e`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

* Refactor preference package * Unit tests * Apply suggestions from code review Co-authored-by: Parthvi Vala <pvala@redhat.com> * Review * Review from Dharmit * Replace panic with LogErrorAndExit * Remove preference from kclient/oc_server * Remove preference.New from devfile * Remove preference.New from kclient/WaitAndGetPodWithEvents * Get prefClient from CreateOptions * Parthvi review Co-authored-by: Parthvi Vala <pvala@redhat.com>

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/cleanup labels Dec 16, 2021

openshift-ci bot requested review from mohammedzee1000 and rnapoles-rh December 16, 2021 09:14

feloy force-pushed the refactor/preference branch from 156fecb to 41d8cc6 Compare December 16, 2021 09:19

feloy changed the title ~~[wip] Separate layers : Preference~~ Separate layers : Preference Dec 16, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Dec 16, 2021

redhat-developer deleted a comment from odo-robot bot Dec 17, 2021

valaparthvi suggested changes Dec 20, 2021

View reviewed changes

openshift-ci bot assigned valaparthvi Dec 20, 2021

dharmit mentioned this pull request Dec 20, 2021

Having separate layers #5247

Closed

15 tasks

feloy force-pushed the refactor/preference branch from d0a7040 to 1540816 Compare December 20, 2021 13:33

feloy force-pushed the refactor/preference branch from 1540816 to ca79149 Compare December 20, 2021 13:40

feloy force-pushed the refactor/preference branch from a454093 to 277a8c3 Compare December 20, 2021 14:17

feloy force-pushed the refactor/preference branch from 01187ab to 78ca051 Compare January 6, 2022 08:33

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

feloy force-pushed the refactor/preference branch from 3a39532 to d998350 Compare January 6, 2022 14:28

feloy requested a review from valaparthvi January 7, 2022 08:35

valaparthvi suggested changes Jan 10, 2022

View reviewed changes

feloy and others added 11 commits January 10, 2022 11:28

Refactor preference package

f4543d8

Unit tests

ccdc84b

Apply suggestions from code review

58b3862

Co-authored-by: Parthvi Vala <pvala@redhat.com>

Review

4b6121f

Review from Dharmit

2113abd

Replace panic with LogErrorAndExit

e8b4160

Remove preference from kclient/oc_server

d9e1bd5

Remove preference.New from devfile

0b8bf14

Remove preference.New from kclient/WaitAndGetPodWithEvents

f6f3117

Get prefClient from CreateOptions

76d3663

Parthvi review

2bfa3da

feloy force-pushed the refactor/preference branch from 40925c8 to 2bfa3da Compare January 10, 2022 10:28

feloy requested a review from valaparthvi January 10, 2022 11:23

valaparthvi approved these changes Jan 10, 2022

View reviewed changes

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

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Jan 10, 2022

openshift-merge-robot merged commit e63773c into redhat-developer:main Jan 10, 2022

rm3l added the area/refactoring Issues or PRs related to code refactoring label Jun 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate layers : Preference #5296

Separate layers : Preference #5296

feloy commented Dec 16, 2021 •

edited

Loading

netlify bot commented Dec 16, 2021 •

edited

Loading

feloy commented Dec 16, 2021

feloy commented Dec 16, 2021

feloy commented Dec 16, 2021

feloy commented Dec 16, 2021

feloy commented Dec 16, 2021

feloy commented Dec 16, 2021

feloy commented Dec 16, 2021

feloy commented Dec 16, 2021

feloy commented Dec 16, 2021

odo-robot bot commented Dec 17, 2021 •

edited

Loading

valaparthvi Dec 17, 2021

feloy commented Dec 20, 2021

odo-robot bot commented Dec 20, 2021 •

edited

Loading

feloy commented Dec 20, 2021

feloy commented Dec 20, 2021

feloy commented Jan 6, 2022

feloy commented Jan 6, 2022

valaparthvi Jan 10, 2022

valaparthvi Jan 10, 2022

valaparthvi Jan 10, 2022

valaparthvi left a comment

valaparthvi Jan 10, 2022

feloy Jan 10, 2022

valaparthvi commented Jan 10, 2022

openshift-ci bot commented Jan 10, 2022

feloy commented Jan 10, 2022

openshift-ci bot commented Jan 10, 2022 •

edited

Loading

Separate layers : Preference #5296

Separate layers : Preference #5296

Conversation

feloy commented Dec 16, 2021 • edited Loading

netlify bot commented Dec 16, 2021 • edited Loading

feloy commented Dec 16, 2021

feloy commented Dec 16, 2021

feloy commented Dec 16, 2021

feloy commented Dec 16, 2021

feloy commented Dec 16, 2021

feloy commented Dec 16, 2021

feloy commented Dec 16, 2021

feloy commented Dec 16, 2021

feloy commented Dec 16, 2021

odo-robot bot commented Dec 17, 2021 • edited Loading

valaparthvi Dec 17, 2021

Choose a reason for hiding this comment

feloy commented Dec 20, 2021

odo-robot bot commented Dec 20, 2021 • edited Loading

feloy commented Dec 20, 2021

feloy commented Dec 20, 2021

feloy commented Jan 6, 2022

feloy commented Jan 6, 2022

valaparthvi Jan 10, 2022

Choose a reason for hiding this comment

valaparthvi Jan 10, 2022

Choose a reason for hiding this comment

valaparthvi Jan 10, 2022

Choose a reason for hiding this comment

valaparthvi left a comment

Choose a reason for hiding this comment

valaparthvi Jan 10, 2022

Choose a reason for hiding this comment

feloy Jan 10, 2022

Choose a reason for hiding this comment

valaparthvi commented Jan 10, 2022

openshift-ci bot commented Jan 10, 2022

feloy commented Jan 10, 2022

openshift-ci bot commented Jan 10, 2022 • edited Loading

feloy commented Dec 16, 2021 •

edited

Loading

netlify bot commented Dec 16, 2021 •

edited

Loading

odo-robot bot commented Dec 17, 2021 •

edited

Loading

odo-robot bot commented Dec 20, 2021 •

edited

Loading

openshift-ci bot commented Jan 10, 2022 •

edited

Loading