refactor + fix: Cleanup proxy bundle #526

akrejcir · 2023-03-16T11:03:38Z

What this PR does / why we need it:

Cleaned up code in vm-console-proxy-bundle module
Added validation that the bundle contains needed all resources

Release note:

None

akrejcir · 2023-03-16T11:03:57Z

/cc @codingben @0xFelix

codingben · 2023-03-16T13:20:01Z

@akrejcir IMHO it's code refactor: and not chore:, I think it'd be better to have the commit message as refactor: cleanup vm-console-proxy bundle. Personally I think that chore means non-code changes.

E.g. chore: Refactor proxy bundle code -> refactor: proxy bundle.

internal/vm-console-proxy-bundle/bundle-test-incorrect.yaml

codingben · 2023-03-16T13:23:51Z

internal/vm-console-proxy-bundle/bundle.go

-	o, err := yamlv2.Marshal(&obj)
-	if err != nil {
-		return err
+func validateBundle(bundle *Bundle) error {


I think validateBundle it's too general naming and validateBundleObjects would be better.

I disagree. It validates that the Bundle struct is correct and can be used.

Yes, but it didn't mention what kind of validation it does. Maybe validateBundleParameters.

There is only one function that does validation on Bundle, so it does all the validation. IMHO it's not needed to be more specific. If there would be multiple functions, then yes.

codingben · 2023-03-16T13:30:13Z

internal/vm-console-proxy-bundle/bundle.go

-				}
-			}
+		err = runtime.DefaultUnstructuredConverter.FromUnstructured(obj.UnstructuredContent(), destObj)
+		if err != nil {


What about make it shorter:

if err := runtime.DefaultUnstructuredConverter.FromUnstructured(obj.UnstructuredContent(), destObj); err != nil { return nil, err }

The line is already quite long. I prefer it this way.

codingben · 2023-03-16T13:32:15Z

internal/vm-console-proxy-bundle/bundle.go

+	for {
+		obj := &unstructured.Unstructured{}
+		err := decoder.Decode(&obj)
+		if err == io.EOF {


If you like it:

if err := decoder.Decode(&obj); err == io.EOF { return bundle, nil }

The err is checked multiple times, and this change would limit its scope only to the inside of the if statement.

akrejcir · 2023-03-17T08:27:17Z

/retest

akrejcir · 2023-03-17T12:03:16Z

/retest

0xFelix

Is it really necessary to "half-hardcode" the expected bundle resources into the operand and then to load their values from the bundle YAML? Can't we either define the resources completely in code or load them just from the YAML?

internal/vm-console-proxy-bundle/bundle.go

0xFelix · 2023-03-17T13:03:30Z

internal/vm-console-proxy-bundle/bundle.go

@@ -91,3 +99,29 @@ func loadBundleFromBytes(data []byte) (*Bundle, error) {
 		}
 	}
 }
+
+func validateBundle(bundle *Bundle) error {
+	missingFields := make([]string, 0, 6)


Do we gain something by explicitly sizing the slice?

We gain a little bit of performance for no change in readability.

internal/vm-console-proxy-bundle/bundle.go

0xFelix · 2023-03-17T13:10:14Z

Also, the commit messages are a bit sparse. Could you give some more context, please?

akrejcir · 2023-03-20T15:53:42Z

Is it really necessary to "half-hardcode" the expected bundle resources into the operand and then to load their values from the bundle YAML? Can't we either define the resources completely in code or load them just from the YAML?

We cannot define the resources in ssp operator code, because they can change when a new version of vm-console-proxy is released. Loading them only from yaml would be good, but the reconciliation code needs to know the types of resources, so it can use the correct update functions.

I think we can think of a more generic solution as part of the SSP cleanup epic.

codingben · 2023-03-20T21:55:04Z

Can you please change PR title from chore: Cleanup proxy bundle to refactor: Cleanup proxy bundle?

codingben

I'd maybe move bundle-test.yaml and bundle-test-incorrect.yaml to test-yamls folder or something. It's up to you.

/lgtm

0xFelix

Added some comments.

We cannot define the resources in ssp operator code, because they can change when a new version of vm-console-proxy is released. Loading them only from yaml would be good, but the reconciliation code needs to know the types of resources, so it can use the correct update functions.

TTO can load resources from the pipeline / task manifests without knowing exactly which resources it will encounter. It only has a list of known kinds it can handle / reconcile. Can't you do the same?

0xFelix · 2023-03-21T09:02:33Z

internal/vm-console-proxy-bundle/bundle.go

+		kind := obj.GetKind()
+
+		var destObj any
+		switch obj.GetKind() {


switch kind := obj.GetKind(); kind?

Or if you need the kind var later, use it here too?

https://github.com/kubevirt/tekton-tasks-operator/blob/main/pkg/tekton-bundle/tekton-bundle.go#L150

@0xFelix Done

@ksimon1 That code was used originally here, and I just refactored it to make it smaller.

internal/vm-console-proxy-bundle/bundle.go

Reading of the vm-console-proxy bundle fails, if the yaml bundle contains an object with unexpected kind. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

Simplify code that loads vm-console-proxy bundle. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

Validate that no field in Bundle is nil. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

akrejcir · 2023-03-21T16:37:55Z

TTO can load resources from the pipeline / task manifests without knowing exactly which resources it will encounter. It only has a list of known kinds it can handle / reconcile. Can't you do the same?

It will be some work, so we can add an Issue.

sonarcloud · 2023-03-21T16:45:51Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

0xFelix

/lgtm

akrejcir · 2023-03-24T12:37:25Z

/approve

kubevirt-bot · 2023-03-24T12:37:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akrejcir

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

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

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Mar 16, 2023

kubevirt-bot requested review from 0xFelix and ksimon1 March 16, 2023 11:03

kubevirt-bot added the size/L label Mar 16, 2023

kubevirt-bot requested a review from codingben March 16, 2023 11:03

akrejcir changed the title ~~Cleanup proxy bundle~~ chore: Cleanup proxy bundle Mar 16, 2023

akrejcir force-pushed the cleanup-proxy-bundle branch from e6aae63 to 8ce3734 Compare March 16, 2023 11:50

codingben reviewed Mar 16, 2023

View reviewed changes

internal/vm-console-proxy-bundle/bundle-test-incorrect.yaml Show resolved Hide resolved

codingben reviewed Mar 16, 2023

View reviewed changes

akrejcir force-pushed the cleanup-proxy-bundle branch 2 times, most recently from 9de100c to 9ccb818 Compare March 16, 2023 16:08

0xFelix reviewed Mar 17, 2023

View reviewed changes

akrejcir force-pushed the cleanup-proxy-bundle branch from 9ccb818 to c0cdaf1 Compare March 20, 2023 15:48

codingben approved these changes Mar 20, 2023

View reviewed changes

kubevirt-bot assigned codingben Mar 20, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 20, 2023

akrejcir changed the title ~~chore: Cleanup proxy bundle~~ refactor + fix: Cleanup proxy bundle Mar 21, 2023

0xFelix reviewed Mar 21, 2023

View reviewed changes

akrejcir added 2 commits March 21, 2023 17:08

fix: vm-console-proxy-bundle checks unsupported Kind

2f73eba

Reading of the vm-console-proxy bundle fails, if the yaml bundle contains an object with unexpected kind. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

refactor: proxy bundle code

4a41a07

Simplify code that loads vm-console-proxy bundle. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

fix: validate that proxy bundle contains all resources

22f6fbe

Validate that no field in Bundle is nil. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

akrejcir force-pushed the cleanup-proxy-bundle branch from c0cdaf1 to 22f6fbe Compare March 21, 2023 16:36

kubevirt-bot added size/XL and removed lgtm Indicates that a PR is ready to be merged. size/L labels Mar 21, 2023

akrejcir mentioned this pull request Mar 21, 2023

Make reconciler code more generic #528

Open

0xFelix approved these changes Mar 24, 2023

View reviewed changes

kubevirt-bot assigned 0xFelix Mar 24, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 24, 2023

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 24, 2023

kubevirt-bot merged commit 37dfdaf into kubevirt:master Mar 24, 2023
3 checks passed

akrejcir deleted the cleanup-proxy-bundle branch March 24, 2023 12:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor + fix: Cleanup proxy bundle #526

refactor + fix: Cleanup proxy bundle #526

akrejcir commented Mar 16, 2023

akrejcir commented Mar 16, 2023

codingben commented Mar 16, 2023 •

edited

codingben Mar 16, 2023

akrejcir Mar 16, 2023

codingben Mar 16, 2023

akrejcir Mar 16, 2023

codingben Mar 16, 2023

akrejcir Mar 16, 2023

codingben Mar 16, 2023

akrejcir Mar 16, 2023

akrejcir commented Mar 17, 2023

akrejcir commented Mar 17, 2023

0xFelix left a comment

0xFelix Mar 17, 2023

akrejcir Mar 20, 2023 •

edited

0xFelix commented Mar 17, 2023

akrejcir commented Mar 20, 2023

codingben commented Mar 20, 2023

codingben left a comment

0xFelix left a comment

0xFelix Mar 21, 2023

0xFelix Mar 21, 2023

ksimon1 Mar 21, 2023

akrejcir Mar 21, 2023

akrejcir commented Mar 21, 2023

sonarcloud bot commented Mar 21, 2023

0xFelix left a comment

akrejcir commented Mar 24, 2023

kubevirt-bot commented Mar 24, 2023

refactor + fix: Cleanup proxy bundle #526

refactor + fix: Cleanup proxy bundle #526

Conversation

akrejcir commented Mar 16, 2023

akrejcir commented Mar 16, 2023

codingben commented Mar 16, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akrejcir commented Mar 17, 2023

akrejcir commented Mar 17, 2023

0xFelix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akrejcir Mar 20, 2023 • edited

Choose a reason for hiding this comment

0xFelix commented Mar 17, 2023

akrejcir commented Mar 20, 2023

codingben commented Mar 20, 2023

codingben left a comment

Choose a reason for hiding this comment

0xFelix left a 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

Choose a reason for hiding this comment

akrejcir commented Mar 21, 2023

sonarcloud bot commented Mar 21, 2023

0xFelix left a comment

Choose a reason for hiding this comment

akrejcir commented Mar 24, 2023

kubevirt-bot commented Mar 24, 2023

codingben commented Mar 16, 2023 •

edited

akrejcir Mar 20, 2023 •

edited