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

Add CustomResourceDefinition (WIP) #149

Merged
merged 8 commits into from
Nov 14, 2017
Merged

Add CustomResourceDefinition (WIP) #149

merged 8 commits into from
Nov 14, 2017

Conversation

fhemberger
Copy link
Contributor

Adds CustomResourceDefinition, which replaces ThirdPartyResources starting Kubernetes 1.7.

This is still work in progress, not sure I got your setup right. Tests are failing at the moment. Would be cool if you could give me a hand with this.

Adds `CustomResourceDefinition`, which replaces `ThirdPartyResources` starting Kubernetes 1.7.
@silasbw
Copy link
Contributor

silasbw commented Oct 4, 2017

@fhemberger thank you for starting on this -- we definitely need it. I'll take a look tomorrow or the next day and see if I can help with the tests!

@fhemberger
Copy link
Contributor Author

@silasbw Great, thank you. I got something wrong with the implementation but couldn't find out what it was … probably I'm just missing something.

Also CRDs can be namespace or cluster specific, I think a part of that is still missing.

Copy link
Contributor

@silasbw silasbw left a comment

Choose a reason for hiding this comment

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

Sorry the delay. Hope the review is still helpful.

class CustomResourceDefinition extends ApiGroup {
constructor(options) {
options = Object.assign({}, options, {
path: options.group,
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 you want (will also "fix" one of your unit tests):

path: `apis/${ options.group }`,

https://kubernetes.io/docs/tasks/access-kubernetes-api/extend-api-custom-resource-definitions/

// improvements to our integration test harness to make this work well.
common.only('unit', 'can POST and GET', done => {
async.series([
next => common.apiExtensions.post({ body: testApiExtensions }, next),
Copy link
Contributor

Choose a reason for hiding this comment

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

You want this:

next => common.apiExtensions.customresourcedefinitions.post({ body: testApiExtensions }, next),
next => common.apiExtensions.customresourcedefinitions.get(testCustomResourceName, next)

https://kubernetes.io/docs/api-reference/v1.8/#-strong-write-operations-strong--254

path: 'apis/apiextensions.k8s.io',
version: options.version || 'v1beta1',
groupResources: [
'customresourcedefinition'
Copy link
Contributor

Choose a reason for hiding this comment

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

pluralize: customresourcedefinitions

The code automatically creates a singular alias so apiExtensions.customresourcedefinitions and apiExtensions.customresourcedefinition will both work, but you need to specify the plural version here.

'customresourcedefinition'
],
namespaceResources: [
{ name: 'customresourcedefinition', Constructor: CustomResourceDefiniton }
Copy link
Contributor

Choose a reason for hiding this comment

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

pluralize here too.


beforeTesting('unit', () => {
nock(common.apiExtensions.url)
.post(`${ common.apiExtensions.path }`)
Copy link
Contributor

Choose a reason for hiding this comment

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

you'll need to update these mocks:

        .post(`${ common.apiExtensions.path }/customresourcedefinitions`)
        .reply(201, testApiExtensions)
        .get(`${ common.apiExtensions.path }/customresourcedefinitions/${ testCustomResourceName }`)
        .reply(200, testApiExtensions);

test/common.js Outdated
@@ -76,6 +79,9 @@ function injectApis(options) {
rbac: { Constructor: Rbac },
thirdPartyResources: {
Constructor: ThirdPartyResources, options: { group: 'kubernetes-client.com' }
},
customResourceDefinition: {
Copy link
Contributor

Choose a reason for hiding this comment

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

pluralize (similar to thirdPartyResources)

@fhemberger
Copy link
Contributor Author

Thanks, but now 4 instead of two tests are failing … 🤔
I need to dig deeper into it.

@silasbw
Copy link
Contributor

silasbw commented Oct 11, 2017

Hey @fhemberger sent you https://github.com/fhemberger/kubernetes-client/pull/1.

unit tests work, integration tests have some issues. I'll take a look at those when i get a moment.

@fhemberger
Copy link
Contributor Author

Ok, back from vacation, updated PR with your changes, @silasbw.
Are we good to go or ist there still something missing?

@silasbw
Copy link
Contributor

silasbw commented Nov 14, 2017

Hey @fhemberger, I sent you some tweaks to fix integration tests, etc:

https://github.com/fhemberger/kubernetes-client/pull/2

Sorry this has been such a slog and thanks for sticking with it. We're working on making these types of contributions much easier to make.

@fhemberger
Copy link
Contributor Author

@silasbw Awesome, tests are passing! 💯

Copy link
Contributor

@silasbw silasbw left a comment

Choose a reason for hiding this comment

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

🏆 !!

@silasbw silasbw merged commit 4261c2e into godaddy:master Nov 14, 2017
@silasbw
Copy link
Contributor

silasbw commented Nov 14, 2017

I published these changes in kubernetes-client@3.17.0. Thanks again.

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

2 participants