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.

Sign up for GitHub

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

Jump to bottom

Generate CRDs with structural schema #1885

Merged
merged 8 commits into from Sep 23, 2019
Merged

Generate CRDs with structural schema #1885

merged 8 commits into from Sep 23, 2019

Conversation

prydonius
Copy link
Contributor

@prydonius prydonius commented Sep 18, 2019 •
edited

This PR introduces CRD generation using the controller-tools tooling. The CRDs are generated to a manifests folder, and a custom code generator is used to embed the manifests in a Go pkg and make them retrievable as API objects.

Signed-off-by: Adnan Abdulhussein aadnan@vmware.com

Adnan Abdulhussein added 4 commits September 17, 2019 16:56
generating CRDs with controller-tools and embedding in binary
d263439 
Signed-off-by: Adnan Abdulhussein <aadnan@vmware.com>
add component=velero labels to generated crds
0aa767a 
Signed-off-by: Adnan Abdulhussein <aadnan@vmware.com>
use top-level validation field for schema
60615cf 
Signed-off-by: Adnan Abdulhussein <aadnan@vmware.com>
remove trivialVersions due to upstream fix
ce1b449 
Signed-off-by: Adnan Abdulhussein <aadnan@vmware.com>
@skriss
Copy link
Member

skriss commented Sep 19, 2019

@prydonius are you looking for input on this yet? I haven't looked yet but can if you're wanting feedback

@prydonius
Copy link
Contributor Author

Thanks @skriss, would be great if you could take a look at the code generation, though so far I'm still testing the schema validation with different clusters, and need to finish making the changes to the optional API fields and push that up.

skriss
skriss reviewed Sep 20, 2019
View reviewed changes
pkg/generated/crds/crds_generate.go Outdated
@@ -0,0 +1,117 @@
// +build ignore
Copy link
Member

Choose a reason for hiding this comment

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

This LGTM - only thing I'm wondering is if we want to relocate this file to be outside of pkg/generated? It could make sense to go in a subdir of hack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was thinking about that too - I think that makes sense, will move it there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this was moved to hack/crd-gen/main.go and this needs to be deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep 😅thanks for the catch!

Adnan Abdulhussein added 2 commits September 20, 2019 10:26
revert velero install change
a2d038b 
Signed-off-by: Adnan Abdulhussein <aadnan@vmware.com>
move crd generator code to hack/
984c65e 
Signed-off-by: Adnan Abdulhussein <aadnan@vmware.com>
@prydonius prydonius marked this pull request as ready for review September 20, 2019 17:45
@prydonius
Copy link
Contributor Author

I'm going to make the API type changes in a different PR as it's just going to be easier to review. The current CRDs are broken due to non-optional status fields, so I've reverted the velero install change to use the non-structural CRDs it was using before. This is now ready for review!

@prydonius
Copy link
Contributor Author

@carlisia ptal if you have a chance!

@prydonius prydonius mentioned this pull request Sep 20, 2019
Merged
carlisia
carlisia approved these changes Sep 20, 2019
View reviewed changes
Copy link
Contributor

@carlisia carlisia left a comment

Choose a reason for hiding this comment

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

A nit + I think a file needs to be removed but else great work figuring this out! 💯

hack/crd-gen/main.go

// This code embeds the CRD manifests in pkg/generated/crds/manifests in
// pkg/generated/crds/crds.go.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I would delete this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, most other files have an empty line between the comment and package name (unless documenting what the package is for), so probably best to keep that consistent?

pkg/generated/crds/crds_generate.go Outdated
@@ -0,0 +1,117 @@
// +build ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this was moved to hack/crd-gen/main.go and this needs to be deleted?

Adnan Abdulhussein added 2 commits September 20, 2019 16:21
remove moved crds generator
0e716f6 
Signed-off-by: Adnan Abdulhussein <aadnan@vmware.com>
add comments about paths relative to pkg dir
e951dc1 
Signed-off-by: Adnan Abdulhussein <aadnan@vmware.com>
skriss
skriss approved these changes Sep 23, 2019
View reviewed changes
Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

LGTM - this should be ready for merge, right?

@prydonius
Copy link
Contributor Author

@skriss yep this can be merged, and I can rebase and update #1898 on top of this

@skriss skriss merged commit 5e22f9c into vmware-tanzu:master Sep 23, 2019
@prydonius prydonius deleted the 1675-poc-embed branch September 23, 2019 16:40
jessestuart added a commit to jessestuart/velero that referenced this pull request Sep 28, 2019
@jessestuart
Merge remote-tracking branch 'upstream/master'
a7b7f05 
* upstream/master: (38 commits)
  sync controller: replace revision file with full diff each interval (vmware-tanzu#1892)
  Increment logging for item backupper (vmware-tanzu#1904)
  Add LD_LIBRARY_PATH as an env varible for the use of vsphere plugin (vmware-tanzu#1893)
  Remove unused flag (vmware-tanzu#1913)
  Use layers in the builder Dockerfile (vmware-tanzu#1907)
  Fix for vmware-tanzu#1888: check item's original namespace, not remapped one, for inclusion/exclusion (vmware-tanzu#1909)
  fail on make verify if generated CRDs differ (vmware-tanzu#1906)
  velero API type changes for structural schema CRDs (vmware-tanzu#1898)
  Generate CRDs with structural schema (vmware-tanzu#1885)
  Plan for moving plugin repos (vmware-tanzu#1870)
  move plugin proto updating into make update (vmware-tanzu#1887)
  Add features package (vmware-tanzu#1849)
  GCP: support specifying Cloud KMS key name for backup storage locations (vmware-tanzu#1879)
  Adds to website (vmware-tanzu#1882)
  proposal for generating Velero CRDs with structural schema (vmware-tanzu#1875)
  Improve contributing docs (vmware-tanzu#1852)
  [doc] Diagram (image) now mentions velero  (vmware-tanzu#1877)
  AWS: add support for arbitrary SSE algorithms, e.g. AES256 (vmware-tanzu#1869)
  update restic docs for PR vmware-tanzu#1807 (vmware-tanzu#1867)
  changelog for PR vmware-tanzu#1864
  ...
@prydonius prydonius mentioned this pull request Oct 4, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlisia carlisia carlisia approved these changes

@skriss skriss skriss approved these changes

Assignees

@carlisia carlisia

@nrb nrb

@skriss skriss

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@prydonius @skriss @carlisia @nrb