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

federation: Adding secret API #29138

Merged
merged 1 commit into from
Aug 5, 2016
Merged

federation: Adding secret API #29138

merged 1 commit into from
Aug 5, 2016

Conversation

kshafiee
Copy link
Contributor

@kshafiee kshafiee commented Jul 18, 2016 •
edited
Loading

Adding secret API to federation-apiserver and updating the federation client to include secrets

@k8s-bot
Copy link

k8s-bot commented Jul 18, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

4 similar comments
@k8s-bot
Copy link

k8s-bot commented Jul 18, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Jul 18, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Jul 18, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Jul 18, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Jul 18, 2016
@ghost
Copy link

ghost commented Jul 18, 2016

ok to test

@ghost
Copy link

ghost commented Jul 18, 2016

@nikhiljindal is back! :-) Could you take a look?

@kshafiee could you please add an e2e test for this (i.e. one that creates, deletes and updates a secret, and confirms that it's appropriately reflected in the API).

Let me know if you're unclear on how to do that. I can send you some pointers.

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 18, 2016
@ghost ghost assigned nikhiljindal and unassigned ghost Jul 19, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2016
@kshafiee
Copy link
Contributor Author

ok to test

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2016
@kshafiee
Copy link
Contributor Author

@quinton-hoole would be great if you can send me the pointers on e2e test, thx

@ghost
Copy link

ghost commented Jul 19, 2016

@kshafiee

e2e test setup

api testing

federated service controller testing

@nikhiljindal nikhiljindal added this to the v1.4 milestone Jul 19, 2016
@nikhiljindal
Copy link
Contributor

@kshafiee You will also need to run hack/update-codegen.sh for it to update the generated clientset.

@nikhiljindal nikhiljindal added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Jul 19, 2016
@kshafiee kshafiee force-pushed the pr-27789 branch 2 times, most recently from a667131 to dd1c55c Compare July 20, 2016 16:32
@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 20, 2016
@nikhiljindal
Copy link
Contributor

nikhiljindal commented Jul 28, 2016 •
edited
Loading

TestAdmissionNamespaceExists failure seems unrelated.

admission_test.go:89: No client request should have been madefrom junit_v1,autoscaling-v1,batch-v1,batch-v2alpha1,extensions-v1beta1,apps-v1alpha1,federation-v1beta1,policy-v1alpha1,rbac.authorization.k8s.io-v1alpha1,certificates-v1alpha1_20160725-134416.xml

@k8s-bot unit test this issue: #IGNORE

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 1, 2016
@kshafiee
Copy link
Contributor Author

kshafiee commented Aug 1, 2016

@nikhiljindal just rebased and added the e2e test file federated-secret.go
Please let me know if this looks better now. thx!

nikhiljindal
nikhiljindal reviewed Aug 2, 2016
View reviewed changes
test/e2e/federated-secret.go
}
})

It("should succeed", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

change the first param to It to "should be created and deleted successfully".

@nikhiljindal
Copy link
Contributor

Thanks @kshafiee the e2e test looks good barring few comments I have added.

Also you seem to have deleted some files from clientset_generated that seems unintentional. Your code should not be compiling without those files (it is failing on jenkins).
Please restore those files.

@nikhiljindal nikhiljindal added area/cluster-federation and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Aug 2, 2016
@nikhiljindal
Copy link
Contributor

Also cc @kubernetes/sig-cluster-federation

@kshafiee
Copy link
Contributor Author

kshafiee commented Aug 2, 2016

This PR resolves #29337

@kshafiee
Copy link
Contributor Author

kshafiee commented Aug 2, 2016 •
edited
Loading

Thanks @nikhiljindal for your comments!
#29937 should remove the need for b9d54a7

@kshafiee
Copy link
Contributor Author

kshafiee commented Aug 2, 2016

cc @sig-cluster-federation

@nikhiljindal
Copy link
Contributor

Great to see the tests pass on Jenkins. Code changes LGTM barring one unresolved comment. Can you please fix that?

Also please squash the commits.
Also can you please confirm that federation e2e tests are passing for you with these changes?
Thanks!

@k8s-bot
Copy link

k8s-bot commented Aug 3, 2016

GCE e2e build/test failed for commit 262ae3d.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@fejta fejta mentioned this pull request Aug 3, 2016
Closed
@fejta
Copy link
Contributor

fejta commented Aug 3, 2016

@k8s-bot e2e test this issue #29966

@k8s-bot
Copy link

k8s-bot commented Aug 3, 2016

GCE e2e build/test passed for commit 262ae3d.

@kshafiee
Copy link
Contributor Author

kshafiee commented Aug 4, 2016

Thanks @nikhiljindal for your comments!

  • I addressed your comment
  • squashed the commits
  • confirmed that federation e2e tests are passing

@nikhiljindal
Copy link
Contributor

Awesome, thanks LGTM

@nikhiljindal nikhiljindal added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 4, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Aug 5, 2016

GCE e2e build/test passed for commit 262ae3d.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 1cd07a2 into kubernetes:master Aug 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nikhiljindal nikhiljindal

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.
7 participants
@kshafiee @k8s-bot @nikhiljindal @colhom @fejta @k8s-github-robot @googlebot