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

Add CRD validation schemata #17

Merged
merged 7 commits into from Mar 5, 2019
Merged

Add CRD validation schemata #17

merged 7 commits into from Mar 5, 2019

Conversation

sttts
Copy link
Contributor

@sttts sttts commented Feb 19, 2019

No description provided.

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 19, 2019
deads2k
deads2k reviewed Feb 19, 2019
View reviewed changes
hack/update-crd-validation/main.go Outdated

func main() {
// load existing manifests from manifests/ dir
existingManifests, err := crdsFromDirectory("manifests")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok starting here. Seems like we should just generate everything though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have to copy this mechanism to all repos that have CRDs.

@deads2k
Copy link
Contributor

deads2k commented Feb 19, 2019

@sttts had to tweak the generator? I can live with that, but maybe a separate commit to make more easily pickable?

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 19, 2019
@damemi damemi mentioned this pull request Feb 19, 2019
Closed
@damemi
Copy link

damemi commented Feb 19, 2019

Do we still want to pursue kubernetes-sigs/controller-tools#144? Could do everything from the makefile without a hack file and a dependency on the controller tools. I'm open to either

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 20, 2019
@sttts
Copy link
Contributor Author

sttts commented Feb 20, 2019

@sttts had to tweak the generator? I can live with that, but maybe a separate commit to make more easily pickable?

No tweaking. I copy files where the generator expects them, and I only copy the validation part. So whatever the generator does outside, we don't care. @damemi's PR against the generator will fix this, so we can make use of more if we want to.

@sttts sttts added the lgtm Indicates that a PR is ready to be merged. label Feb 20, 2019
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 20, 2019
@sttts sttts changed the title Quick'n'dirty kubebuilder CRD validations for config.openshift.io Add CRD validation schemata Feb 20, 2019
@sttts sttts force-pushed the sttts-crd-validation branch 2 times, most recently from 9d222dc to cbfc3c8 Compare February 20, 2019 11:20
soltysh
soltysh approved these changes Feb 20, 2019
View reviewed changes
Copy link
Member

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm

Makefile Outdated
@@ -31,3 +31,14 @@ $(call add-bindata,v3.11.0,./bindata/v3.11.0/...,bindata,v311_00_assets,pkg/oper
clean:
$(RM) ./cluster-config-operator
.PHONY: clean

update-codegen:
Copy link
Member

Choose a reason for hiding this comment

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

Don't you want to move that to library-go?

Copy link
Member

Choose a reason for hiding this comment

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

@tnozicka fyi

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 20, 2019
tnozicka
tnozicka suggested changes Feb 20, 2019
View reviewed changes
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 20, 2019
@sttts sttts force-pushed the sttts-crd-validation branch 3 times, most recently from 8897700 to 5b91af9 Compare February 20, 2019 14:02
@sttts
Copy link
Contributor Author

sttts commented Feb 22, 2019

/retest

9 similar comments
@sttts
Copy link
Contributor Author

sttts commented Feb 22, 2019

/retest

@sttts
Copy link
Contributor Author

sttts commented Feb 22, 2019

/retest

@sttts
Copy link
Contributor Author

sttts commented Feb 22, 2019

/retest

@sttts
Copy link
Contributor Author

sttts commented Feb 22, 2019

/retest

@sttts
Copy link
Contributor Author

sttts commented Feb 22, 2019

/retest

@sttts
Copy link
Contributor Author

sttts commented Feb 22, 2019

/retest

@sttts
Copy link
Contributor Author

sttts commented Feb 22, 2019

/retest

@sttts
Copy link
Contributor Author

sttts commented Feb 23, 2019

/retest

@sttts
Copy link
Contributor Author

sttts commented Feb 25, 2019

/retest

@damemi damemi mentioned this pull request Feb 27, 2019
Closed
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2019
@sttts sttts force-pushed the sttts-crd-validation branch 3 times, most recently from bd2a6d7 to 24ea0a8 Compare March 5, 2019 14:34
@sttts sttts added the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2019
@deads2k deads2k added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 5, 2019
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: deads2k, soltysh, sttts

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

@deads2k
Copy link
Contributor

deads2k commented Mar 5, 2019

nothing has merged, looks like tide doesn't see approved. merging

@deads2k deads2k merged commit e729153 into openshift:master Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deads2k deads2k deads2k left review comments

@tnozicka tnozicka tnozicka requested changes

@soltysh soltysh soltysh approved these changes

@adambkaplan adambkaplan Awaiting requested review from adambkaplan

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@soltysh soltysh

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@sttts @deads2k @damemi @openshift-ci-robot @soltysh @tnozicka