-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Basic CRDs related to schedule based backup #1
base: main
Are you sure you want to change the base?
Basic CRDs related to schedule based backup #1
Conversation
build/Makefile.generate_proto
Outdated
@@ -0,0 +1,84 @@ | |||
# Copyright 2022 The SODA Authors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, The script generates grpc boilerplate code. It can be moved out from this PR as it is not used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep thought of doing later, any how removed it
build/Makefile
Outdated
@@ -0,0 +1,181 @@ | |||
# Copyright 2022 The SODA Authors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Copyright 2022 The SODA Authors." -> "Copyright 2023 The SODA Authors."
Also please handle in "boilerplate.go.txt"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
) | ||
|
||
// ReclaimPolicy tells about reclamation of the backup. It can be either delete or retain | ||
type BackupScheduleReclaimPolicyType struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, BackupScheduleReclaimPolicyType
can be string
instead of struct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
// Valid values are: | ||
// - "Allow": allows Backups to run concurrently; | ||
// - "Forbid"(default): forbids concurrent runs, skipping next run if previous run hasn't finished yet. | ||
// +optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add default value for ConcurrentPolicy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
|
||
// validates the DailyPolicy | ||
func (d *DailyPolicy) Validate() error { | ||
err := d.CheckTimeFormat() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can return error here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
Dockerfile
Outdated
@@ -0,0 +1,21 @@ | |||
# Copyright 2022 The SODA Authors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Copyright 2022 The SODA Authors." -> "Copyright 2023 The SODA Authors."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
// +genclient | ||
// +kubebuilder:object:root=true | ||
// +kubebuilder:resource:scope=Namespaced | ||
// +kubebuilder:subresource:status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMo, SchedulePolicy
does not maintain status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will not have any impact, but any how changed
cmd/controller-manager/main.go
Outdated
@@ -0,0 +1,21 @@ | |||
/* | |||
Copyright 2022 The SODA Authors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Copyright 2022 The SODA Authors." -> "Copyright 2023 The SODA Authors."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
controllers/doc.go
Outdated
@@ -0,0 +1,17 @@ | |||
/* | |||
Copyright 2022 The SODA Authors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Copyright 2022 The SODA Authors." -> "Copyright 2023 The SODA Authors."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
- apiGroups: ["kahu.io"] | ||
resources: ["backups", "backups/status"] | ||
verbs: ["get", "list", "watch", "patch", "update", "delete"] | ||
- apiGroups: ["*"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, This access might not be required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
resourceNames: ["kahu-manager"] | ||
resources: ["leases"] | ||
verbs: ["get", "list", "watch", "create", "update", "delete"] | ||
- apiGroups: [ "rbac.authorization.k8s.io" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, This access might not be required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
hack/go-format.sh
Outdated
#!/bin/bash | ||
# | ||
# Copyright 2023 The SODA Authors. | ||
# Copyright 2017 the Velero contributors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
multiple copyright attributes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
9fa7908
to
ed41b54
Compare
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #
Test Report Added?:
Test Report:
Special notes for your reviewer: Basic CRDs an generated code, build,compile etc