fix: clean up code around velero server flags CLI command #1343

mateusoliveira43 · 2024-02-25T16:04:08Z

Description

This PR should be fix duplication, missing fields and wrong descriptions around velero server flags code.

What was done

I was checking for wrong descriptions in our CRDs and then came across the velero server flags code. I tried to remove duplication and add TODOs around missing fields. As discussed, that code should be automated: each time velero is updated in go.mod

a check should be run to check if all fields from velero help server are present in DPA (maybe skip some, but add why in code as comments)
and if their descriptions are correct (with some auto corrections).

How to test

Check if the removal of duplication around velero server flags code did not change the behavior of the args passed to velero container.

TODO

This PR SHOULD NOT be merged. It does to many things. Break up in smaller ones.

Signed-off-by: Mateus Oliveira <[email protected]>

openshift-ci · 2024-02-25T16:04:13Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2024-02-25T16:04:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mateusoliveira43

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

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

mateusoliveira43 · 2024-02-25T16:05:22Z

api/v1alpha1/oadp_types.go

+	// --default-snapshot-move-data                          Move data by default for all snapshots supporting data movement.
+	// VeleroConfig.DefaultSnapshotMoveData
+
+	// TODO --default-volume-snapshot-locations mapStringString   List of unique volume providers and default volume snapshot location (provider1:location-01,provider2:location-02,...)


Are any of this TODO fields important and should be added?

mateusoliveira43 · 2024-02-25T16:06:50Z

api/v1alpha1/oadp_types.go

 // AutoCorrect is a collection of auto-correction functions for the DPA CR
 // These auto corrects are in-memory only and do not persist to the CR
 // There should not be another place where these auto-corrects are done
 func (dpa *DataProtectionApplication) AutoCorrect() {
+	// TODO error instead of changing user object?


@shawn-hurley I think you once said we should not change values. Should we change this to be in validator or it is ok?

mateusoliveira43 · 2024-02-25T16:09:10Z

controllers/velero.go

 		// last label overrides previous ones
 		install.WithLabels(veleroDeployment.Labels),
 		// use WithSecret false even if we have secret because we use a different VolumeMounts and EnvVars
 		// see: https://github.com/vmware-tanzu/velero/blob/ed5809b7fc22f3661eeef10bdcb63f0d74472b76/pkg/install/deployment.go#L223-L261
 		// our secrets are appended to containers/volumeMounts in credentials.AppendPluginSpecificSpecs function
 		install.WithSecret(false),
 		install.WithServiceAccountName(common.Velero),
+		// TODO remove
+		install.WithFeatures(dpa.Spec.Configuration.Velero.FeatureFlags),
+		install.WithUploaderType(uploaderType),


this flag is never passed to velero container, because we override it afterwards. Should it be added?

mateusoliveira43 · 2024-02-25T16:09:27Z

controllers/velero.go

@@ -313,12 +306,14 @@ func (r *DPAReconciler) customizeVeleroDeployment(dpa *oadpv1alpha1.DataProtecti
 	}

 	var veleroContainer *corev1.Container
+	// TODO did not understand this


what this part does?

mateusoliveira43 · 2024-02-25T16:11:26Z

pkg/velero/server/args.go

 		}
-		args = append(args, fmt.Sprintf("--log-level=%s", logrusLevel.String()))
+		// TODO should not error out?


if user puts wrong string in restic or node agent timeout, code uses default and does not tell nothing to user

mateusoliveira43 · 2024-02-25T16:12:46Z

pkg/velero/server/args.go

why do we have duplication of fields around velero server flags? some in VeleroConfig, some in VeleroConfig.Args

See discussion for context

kaovilai · 2024-02-27T20:07:53Z

pkg/common/common.go

-}
+// Labels
+const (
+	OadpOperatorLabel       = "openshift.io/oadp"


Can you move this outside package common? I think this would make importing github/oadp/pkg/common add a lot of indirect imports for QE if they are using this label.

Could be pkg/common/labels

api package via _types.go was nice cause imports there is minimal

I think it has more imports than this file, but creating something like Common/consts.go (and Common/functions.go) would make this file have no imports

imports are based on package level too.. so it has to be a different subdir to eliminate deps

common/consts/labels.go

kaovilai · 2024-02-27T20:19:25Z

controllers/dpa_controller.go

+// Conditions
+const (
+	conditionReconciled      = "Reconciled"
+	reconciledReasonComplete = "Complete"
+	reconciledReasonError    = "Error"
+	reconcileCompleteMessage = "Reconcile complete"
+)


I would also move this back to types so it's easier to import without affecting importer's go.mod

kaovilai · 2024-02-27T20:23:17Z

pkg/velero/server/args.go

+	// TODO --kubeconfig
+	// TODO --kubecontext


No we won't allow kubeconfig or kubecontext. Not a todo. Comment previously was to note what was omitted.

comment was it was set elsewhere, where is it set?

There's no need to set these as velero will responds to current cluster and current context is the velero namespace. There's no point specifying these

weshayutin · 2024-02-27T20:24:02Z

@mateusoliveira43 I like the thought, but this will have to wait for 1.4.
I'm going to place a hold so that we're not confused with regards to what we're working on atm.

weshayutin · 2024-02-27T20:24:25Z

/hold

weshayutin · 2024-03-01T14:41:10Z

for our reference: https://wiki.openstack.org/wiki/GitCommitMessages

openshift-merge-robot · 2024-03-01T14:41:26Z

PR needs rebase.

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.

openshift-bot · 2024-05-31T01:00:33Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

mateusoliveira43 · 2024-05-31T12:35:58Z

/lifecycle frozen

openshift-ci · 2024-05-31T12:35:59Z

@mateusoliveira43: The lifecycle/frozen label cannot be applied to Pull Requests.

In response to this:

/lifecycle frozen

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-sigs/prow repository.

mateusoliveira43 · 2024-05-31T12:36:16Z

/remove-lifecycle stale

openshift-ci · 2024-06-19T14:58:40Z

@mateusoliveira43: 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/4.16-images	`6b8638a`	link	true	`/test 4.16-images`
ci/prow/4.16-e2e-test-kubevirt-aws	`6b8638a`	link	true	`/test 4.16-e2e-test-kubevirt-aws`
ci/prow/4.15-e2e-test-kubevirt-aws	`6b8638a`	link	true	`/test 4.15-e2e-test-kubevirt-aws`
ci/prow/4.15-ci-index	`6b8638a`	link	true	`/test 4.15-ci-index`
ci/prow/4.15-e2e-test-aws	`6b8638a`	link	true	`/test 4.15-e2e-test-aws`
ci/prow/4.15-images	`6b8638a`	link	true	`/test 4.15-images`
ci/prow/4.16-e2e-test-aws	`6b8638a`	link	true	`/test 4.16-e2e-test-aws`
ci/prow/4.16-ci-index	`6b8638a`	link	true	`/test 4.16-ci-index`

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

openshift-bot · 2024-09-18T01:00:51Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

mateusoliveira43 · 2024-09-20T17:10:38Z

/remove-lifecycle stale
discuss in scrum

fix: clean up code around velero server flags CLI command

6b8638a

Signed-off-by: Mateus Oliveira <[email protected]>

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 25, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 25, 2024

mateusoliveira43 commented Feb 25, 2024

View reviewed changes

kaovilai reviewed Feb 27, 2024

View reviewed changes

weshayutin removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 27, 2024

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 27, 2024

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 1, 2024

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 31, 2024

openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 31, 2024

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 18, 2024

openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 20, 2024

mateusoliveira43 closed this Sep 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: clean up code around velero server flags CLI command #1343

fix: clean up code around velero server flags CLI command #1343

mateusoliveira43 commented Feb 25, 2024

openshift-ci bot commented Feb 25, 2024

openshift-ci bot commented Feb 25, 2024

mateusoliveira43 Feb 25, 2024

mateusoliveira43 Feb 25, 2024

mateusoliveira43 Feb 25, 2024

mateusoliveira43 Feb 25, 2024

mateusoliveira43 Feb 25, 2024

mateusoliveira43 Feb 25, 2024

kaovilai Feb 27, 2024

kaovilai Feb 27, 2024 •

edited

Loading

kaovilai Feb 27, 2024

mateusoliveira43 Feb 28, 2024

kaovilai Feb 28, 2024 •

edited

Loading

kaovilai Feb 28, 2024

kaovilai Feb 27, 2024

kaovilai Feb 27, 2024

mateusoliveira43 Feb 28, 2024

kaovilai Feb 28, 2024

weshayutin commented Feb 27, 2024

weshayutin commented Feb 27, 2024

weshayutin commented Mar 1, 2024

openshift-merge-robot commented Mar 1, 2024

openshift-bot commented May 31, 2024

mateusoliveira43 commented May 31, 2024

openshift-ci bot commented May 31, 2024

mateusoliveira43 commented May 31, 2024

openshift-ci bot commented Jun 19, 2024

openshift-bot commented Sep 18, 2024

mateusoliveira43 commented Sep 20, 2024

fix: clean up code around velero server flags CLI command #1343

fix: clean up code around velero server flags CLI command #1343

Conversation

mateusoliveira43 commented Feb 25, 2024

Description

What was done

How to test

TODO

openshift-ci bot commented Feb 25, 2024

openshift-ci bot commented Feb 25, 2024

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

kaovilai Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaovilai Feb 28, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weshayutin commented Feb 27, 2024

weshayutin commented Feb 27, 2024

weshayutin commented Mar 1, 2024

openshift-merge-robot commented Mar 1, 2024

openshift-bot commented May 31, 2024

mateusoliveira43 commented May 31, 2024

openshift-ci bot commented May 31, 2024

mateusoliveira43 commented May 31, 2024

openshift-ci bot commented Jun 19, 2024

openshift-bot commented Sep 18, 2024

mateusoliveira43 commented Sep 20, 2024

kaovilai Feb 27, 2024 •

edited

Loading

kaovilai Feb 28, 2024 •

edited

Loading