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

Token review endpoint #29796

Merged
merged 3 commits into from
Aug 4, 2016
Merged

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Jul 29, 2016

Unrevert of #28788, which was rolled back because of #29375

@cjcullen @wojtek-t I'd like to remerge if possible. Have we gotten the field checking mentioned here relaxed? #28788 (comment)

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/new-api size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 29, 2016
@wojtek-t
Copy link
Member

@deads2k - I wasn't involved in fixing it, to please don't block on me at all.

@deads2k deads2k assigned cjcullen and unassigned lavalamp Jul 29, 2016
@deads2k deads2k added this to the v1.4 milestone Jul 29, 2016
@cjcullen
Copy link
Member

@deads2k We're still working on the actual relaxation of field checking. As a workaround, I can shove all of the added fields into our server's expected request body, but that's really gross. I know this is holding you guys up right now, so I'll have an answer early next week.

@deads2k
Copy link
Contributor Author

deads2k commented Jul 30, 2016

I know this is holding you guys up right now, so I'll have an answer early next week.

Thanks. I suspect we'll have a similar problem in #20573 when I rebase that one.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 2, 2016
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 2, 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 2, 2016
@deads2k deads2k force-pushed the token-review branch 2 times, most recently from 78982dc to 39c3607 Compare August 2, 2016 17:14
@deads2k
Copy link
Contributor Author

deads2k commented Aug 2, 2016

@k8s-bot unit test this: issue #28713

@deads2k deads2k force-pushed the token-review branch 2 times, most recently from d93d8b8 to c009a9a Compare August 2, 2016 20:02
@deads2k
Copy link
Contributor Author

deads2k commented Aug 3, 2016

kicking tests now that #29375 is resolved.

@deads2k
Copy link
Contributor Author

deads2k commented Aug 3, 2016

GKE tests look to be fixed, smoke test passed with matching levels, #28788 was already tagged, so applying the tag here.

The release note was done for the original pull that was reverted

@deads2k deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 3, 2016
@deads2k deads2k added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Aug 3, 2016
@cjcullen cjcullen removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 3, 2016
@cjcullen
Copy link
Member

cjcullen commented Aug 3, 2016

Sorry @deads2k this is still broken on our side. I'm trying to get the fix pushed out today, but GKE smoke tests still don't quite test the same config as gke-ci.

@deads2k
Copy link
Contributor Author

deads2k commented Aug 3, 2016

Sorry @deads2k this is still broken on our side. I'm trying to get the fix pushed out today, but GKE smoke tests still don't quite test the same config as gke-ci.

Yikes! Is there another issue I can watch? I saw the existing one get resolved.

@cjcullen
Copy link
Member

cjcullen commented Aug 3, 2016

@deads2k
Copy link
Contributor Author

deads2k commented Aug 3, 2016

kubernetes/test-infra#332

Thanks, once that's merged, I just have to rekick the tests and this and #20573 will be safe?

@cjcullen
Copy link
Member

cjcullen commented Aug 3, 2016

Now that that's merged, the GKE smoke tests should fail for both this and #20573...

Once I get my GKE server side change rolled out (hopefully soonish today), then GKE smoke tests will start passing for you and I can get out of your way 😃

@cjcullen
Copy link
Member

cjcullen commented Aug 4, 2016

This should be unblocked on our end now. I just manually tested a kubernetes CI build with this change ( 1.4.0-alpha.1.564+8ead63f1270455) and confirmed that authentication is working.

I'm attempting to kick GKE smoke tests just to verify.

@cjcullen
Copy link
Member

cjcullen commented Aug 4, 2016

@k8s-bot e2e test this issue #IGNORE

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

k8s-bot commented Aug 4, 2016

GCE e2e build/test passed for commit d505063.

@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 4, 2016

GCE e2e build/test passed for commit d505063.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 544851a into kubernetes:master Aug 4, 2016
@cjcullen
Copy link
Member

cjcullen commented Aug 4, 2016

Our 6:00 PM automatic deployment pushed a slightly too-old build to our test endpoint, so this is going to appear to have broken things again until I get a fixed build back live... 😒

@wojtek-t
Copy link
Member

wojtek-t commented Aug 4, 2016

@cjcullen - it seems that we hit this also in staging:
http://kubekins.dls.corp.google.com:8080/view/Scalability/job/kubernetes-e2e-gke-large-cluster/67/console

When do you think it will be fixed?

@cjcullen
Copy link
Member

cjcullen commented Aug 4, 2016

@wojtek-t 😞 I didn't realize we had any CI running against our staging endpoint. I'll work on getting an updated build pushed out to staging today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants