-
Notifications
You must be signed in to change notification settings - Fork 38
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
OADP-205: Implement registry inside openshift-velero-plugin #145
Conversation
98e0f4f
to
fe43fa0
Compare
remove uid from internalRegistrySystemContext discovery function fix bslNameForBackup map reuse common.GetBackup get rid of for loop in GetBackup call rest.InClusterConfig() only once wip plugin registry Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com> wip Use strconv.ParseBool bump logrusr to v3 to accomodate logr v1.2 improve imagestream logging resource name fix more logs is-restore typo more log error strings for registryenv make space for decodedByte error returns secret name and encoded data data already decoded? add logger to GetUdistributionTransportForLocation fix getRegistryEnvsForLocation make map
8224e75
to
06a5453
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
a40559b
to
e50ce3f
Compare
Resolved outstanding issues.
|
github.com/hashicorp/go-plugin v1.3.0 // indirect | ||
github.com/openshift/api v0.0.0-20210105115604-44119421ec6b | ||
github.com/kaovilai/udistribution v0.0.5 |
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.
Should this point to somewhere other than your personal repo?
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.
I guess that's where the project lives right now -- i.e. it's not a fork. Should it be moved to openshift and/or konveyor though, since it's to be part of a supported product?
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.
+1 we should move this to Konveyor at some point. Indifferent on when right now. Can live in kaovilai until another project uses it.
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.
LGTM
Just noticed (again?).. currently registry secret the plugin is fetching from is created from OADP-Operator. Could also move secret parsing to openshift-velero-plugin if wanted in the future. |
velero-plugins/imagestream/backup.go
Outdated
} else { | ||
annotations[common.MigrationRegistry] = backupRegistryRoute | ||
backupRegistryRoute, err := getOADPRegistryRoute(backup.GetUID(), backup.Namespace, backup.Spec.StorageLocation, common.RegistryConfigMap) |
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.
@kaovilai We do not need to fetch the route for OADP, right ?
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.
This is for backwards compatibility such as if we make 1.0.4 release that do not activate the plugin-registry behavior.
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.
We shouldn't need to worry about this since a 1.0.4 would be built off oadp-1.0
branch. It doesn't hurt to have this but I would be in favor of removing it.
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.
ok removing.
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.
Awesome Job @kaovilai, Thank you ! Added some comments.
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.
Overall LGTM, added a couple comments but nothing blocking merge
@kaovilai I do like the idea of enhancing this to do the parsing in the plugin rather than the operator. Something to explore as an enhancement. |
manual test on aws passing. merging now. |
Design doc
openshift/oadp-operator#737
OADP Implementaiton
openshift/oadp-operator#743
To test this PR, you will need to install operator from openshift/oadp-operator#743
with the following command
Use dpa with unsupportedOverrides
Notable changes:
openshift-velero-plugin/velero-plugins/imagestream/restore.go
Lines 49 to 56 in 1600327
openshift-velero-plugin/velero-plugins/imagestream/shared.go
Lines 107 to 121 in 1600327
Setting transport name for containers/image/copy
openshift-velero-plugin/velero-plugins/imagecopy/imagestream.go
Lines 93 to 116 in 1600327
OADP-205