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

s/Operator/OperatorVersion/g in the description #1155

Merged
merged 8 commits into from
Dec 12, 2019
Merged

Conversation

porridge
Copy link
Member

@porridge porridge commented Dec 10, 2019

What this PR does / why we need it:

This is the OperatorVersion object, so the description should mention that object, and not the Operator object.

Also, improved the condition for the generation logic and fixed error handling there, improved a message, and added a README.md for future generations.

This is the `OperatorVersion` object, so the description should mention that object, and not the `Operator` object.
@porridge
Copy link
Member Author

=== RUN   TestCrds_Config
--- FAIL: TestCrds_Config (0.01s)
    init_integration_test.go:101: 
        	Error Trace:	init_integration_test.go:101
        	            				init_integration_test.go:77
        	Error:      	Not equal: 
        	            	expected: "apiVersion: apiextensions.k8s.io/v1beta1\nkind: CustomResourceDefinition\nmetadata:\n  creationTimestamp: null\n  labels:\n    app: kudo-manager\n    controller-tools.k8s.io: \"1.0\"\n  name: instances.kudo.dev\nspec:\n  group: kudo.dev\n  names:\n    kind: Instance\n    plural: instances\n    singular: instance\n  scope: Namespaced\n  subresources:\n    status: {}\n  validation:\n    openAPIV3Schema:\n      properties:\n        apiVersion:\n          type: string\n        kind:\n          type: string\n        metadata:\n          type: object\n        spec:\n          properties:\n            OperatorVersion:\n              description: Operator specifies a reference to a specific Operator object\n              type: object\n            parameters:\n              type: object\n          type: object\n        status:\n          properties:\n            aggregatedStatus:\n              type: object\n            planStatus:\n              type: object\n          type: object\n      type: object\n  version: v1beta1\nstatus:\n  acceptedNames:\n    kind: \"\"\n    plural: \"\"\n  conditions: []\n  storedVersions: []\n"
        	            	actual  : "apiVersion: apiextensions.k8s.io/v1beta1\nkind: CustomResourceDefinition\nmetadata:\n  creationTimestamp: null\n  labels:\n    app: kudo-manager\n    controller-tools.k8s.io: \"1.0\"\n  name: instances.kudo.dev\nspec:\n  group: kudo.dev\n  names:\n    kind: Instance\n    plural: instances\n    singular: instance\n  scope: Namespaced\n  subresources:\n    status: {}\n  validation:\n    openAPIV3Schema:\n      properties:\n        apiVersion:\n          type: string\n        kind:\n          type: string\n        metadata:\n          type: object\n        spec:\n          properties:\n            OperatorVersion:\n              description: OperatorVersion specifies a reference to a specific OperatorVersion object\n              type: object\n            parameters:\n              type: object\n          type: object\n        status:\n          properties:\n            aggregatedStatus:\n              type: object\n            planStatus:\n              type: object\n          type: object\n      type: object\n  version: v1beta1\nstatus:\n  acceptedNames:\n    kind: \"\"\n    plural: \"\"\n  conditions: []\n  storedVersions: []\n"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -29,3 +29,3 @@
        	            	             OperatorVersion:
        	            	-              description: Operator specifies a reference to a specific Operator object
        	            	+              description: OperatorVersion specifies a reference to a specific OperatorVersion object
        	            	               type: object
        	Test:       	TestCrds_Config
        	Messages:   	manifest file does not match the existing one

Ah, so these are auto-generated from somewhere? Let me see if I can figure out where from, and add a warning for future generations...

@porridge
Copy link
Member Author

OK, I found config/crds, pkg/kudoctl/cmd/testdata/deploy-kudo.yaml.golden and friends, as well as pkg/kudoctl/cmd/init/crds.go, but what is the preferred way and order of generating them from each other? @alenkacz you seem to be the expert here, can you point me to where this is documented?

@alenkacz
Copy link
Contributor

@porridge yes! The tests I wrote finally caught something because I was about to reject this PR exactly because of the reason it's failing 🎉

The source of all our CRDs is kudo init so crds.go. So if you want to update anything around CRDs, you need to do it there. We're still keeping the configs also in config/crds because test harness expects folder with CRDs to install when it starts so I wrote a test to make sure these places will always be up to date.

TLDR: Update crds.go and then make all tests pass, then you'll be OK :)

Some issues as a background #798 #977

Copy link
Contributor

@alenkacz alenkacz left a comment

Choose a reason for hiding this comment

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

we need to update crds.go

@porridge
Copy link
Member Author

porridge commented Dec 10, 2019

Update crds.go and then make all tests pass

Right, @alenkacz I understand that crds.go is the authoritative definition. But my question was how do I update the other pieces? Are you saying that there is no automation for that and it needs to be done by hand-editing each file?! 😱

@alenkacz
Copy link
Contributor

@porridge no, take a look at those tests implementation, they always inside contains a way how to update those files :)

@porridge
Copy link
Member Author

PTAL, I changed this in the proper place now, re-generated the golden files and manifests, improved the condition for the generation logic and fixed error handling there, improved a message, and added a README for future generations.

@porridge
Copy link
Member Author

@alenkacz friendly ping ☝️

Copy link
Contributor

@alenkacz alenkacz left a comment

Choose a reason for hiding this comment

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

Nice, thanks for the readme!

@porridge porridge merged commit 3556419 into master Dec 12, 2019
@porridge porridge deleted the porridge-patch-2 branch December 12, 2019 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants