Skip to content

Conversation

@anand99
Copy link
Contributor

@anand99 anand99 commented Aug 13, 2019

Description of changes:
This change includes support for namespaces. It will allow aws-eks-cluster-controller to create namespaces in remote EKS cluster.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@anand99 anand99 requested a review from Magizhchi August 14, 2019 15:21
Copy link
Contributor

@Magizhchi Magizhchi left a comment

Choose a reason for hiding this comment

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

Just a minor suggestion, LGTM to anyway.

g.Expect(fetched).To(gomega.Equal(updated))

// Test Delete
g.Expect(c.Delete(context.TODO(), fetched)).NotTo(gomega.HaveOccurred())
Copy link
Contributor

Choose a reason for hiding this comment

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

NitPick: Would it be better to replace .NotTo(.HaveOccurred()) with .Should(Succeed()). IMO that would improve readability. I would leave either choice at your discretion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would keep same to match testing flow with other components we have for sake of uniformity.

- update
- patch
- delete
- apiGroups:
Copy link
Contributor

Choose a reason for hiding this comment

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

Just Curious, any idea why these roles are removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

- apiGroups:
- apps
resources:
- deployments
verbs:
- get
- list
- watch
- create
- update
- patch
- delete
- apiGroups:
- apps
resources:
- deployments/status
verbs:
- get
- update
- patch

@anand99 anand99 merged commit 7a531f5 into awslabs:master Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants