Skip to content
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

backup: init backup controller #394

Merged
merged 3 commits into from
Oct 18, 2023

Conversation

Xieql
Copy link
Contributor

@Xieql Xieql commented Oct 12, 2023

What type of PR is this?

/kind feature

What this PR does / why we need it:

init the Backup controller.

in addition, refactor the client.go to get client.Client from sigs.k8s.io/controller-runtime/pkg/client

part of #377

part of #374

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

init the Backup controller. 

@netlify
Copy link

netlify bot commented Oct 12, 2023

Deploy Preview for kurator-dev canceled.

Name Link
🔨 Latest commit 1d4ab8b
🔍 Latest deploy log https://app.netlify.com/sites/kurator-dev/deploys/652f3df5c2d9d80008efd9c2

@Xieql Xieql force-pushed the init-backup-controller branch 2 times, most recently from f05c19b to 77c5619 Compare October 12, 2023 08:34
@Xieql Xieql force-pushed the init-backup-controller branch from 77c5619 to 139f0d2 Compare October 12, 2023 08:57
@Xieql
Copy link
Contributor Author

Xieql commented Oct 13, 2023

@hzxuzhonghu PTAL

go.mod Show resolved Hide resolved
pkg/client/client.go Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

nit: prefer move it in backup pkg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

next pr

pkg/fleet-manager/backup_controller.go Outdated Show resolved Hide resolved
pkg/fleet-manager/backup_controller.go Outdated Show resolved Hide resolved
pkg/fleet-manager/backup_controller.go Show resolved Hide resolved
pkg/fleet-manager/backup_controller.go Show resolved Hide resolved
pkg/fleet-manager/backup_restore_migrate_shared.go Outdated Show resolved Hide resolved
return len(backup.Spec.Schedule) != 0
}

func generateVeleroInstanceLabel(createdByLabel, creatorName, fleetName string) map[string]string {
Copy link
Member

Choose a reason for hiding this comment

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

nit: we have many duplicate calling these

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could you please clarify what you mean by 'many duplicate calling these'? Can you point to specific instances or suggest how I might address this?

Copy link
Member

Choose a reason for hiding this comment

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

I mean we set labels in many different places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactor them in subsequent PRs.

pkg/fleet-manager/backup_controller.go Outdated Show resolved Hide resolved
@Xieql
Copy link
Contributor Author

Xieql commented Oct 16, 2023

@hzxuzhonghu

@Xieql Xieql force-pushed the init-backup-controller branch 4 times, most recently from cde0da7 to ee38a5a Compare October 16, 2023 09:34
@Xieql Xieql force-pushed the init-backup-controller branch from ee38a5a to ddbf7a7 Compare October 16, 2023 09:40
pkg/fleet-manager/backup_restore_migrate_shared.go Outdated Show resolved Hide resolved
pkg/fleet-manager/backup_restore_migrate_shared.go Outdated Show resolved Hide resolved
return len(backup.Spec.Schedule) != 0
}

func generateVeleroInstanceLabel(createdByLabel, creatorName, fleetName string) map[string]string {
Copy link
Member

Choose a reason for hiding this comment

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

I mean we set labels in many different places

@Xieql Xieql force-pushed the init-backup-controller branch from f0dea62 to 1d4ab8b Compare October 18, 2023 02:07
@Xieql
Copy link
Contributor Author

Xieql commented Oct 18, 2023

/label tide/merge-method-squash

@Xieql
Copy link
Contributor Author

Xieql commented Oct 18, 2023

@hzxuzhonghu

@hzxuzhonghu
Copy link
Member

Please donot force push, so we can review the delta commit. When it is ready, we should squash then

@Xieql
Copy link
Contributor Author

Xieql commented Oct 18, 2023

Please donot force push, so we can review the delta commit. When it is ready, we should squash then

In fact, this force submission made the delta commit more clear.

I won't force a submission casually.

Copy link
Member

@hzxuzhonghu hzxuzhonghu left a comment

Choose a reason for hiding this comment

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

/lgtm

We should add tests for it

@kurator-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzxuzhonghu

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:

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

@kurator-bot kurator-bot merged commit cc5148b into kurator-dev:main Oct 18, 2023
8 checks passed
@hzxuzhonghu
Copy link
Member

It will help the reviewers to review only the delta change

@Xieql
Copy link
Contributor Author

Xieql commented Oct 18, 2023

/lgtm

We should add tests for it

ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants