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

[WIP] fix TPR single group, multiple versions issue #36157

Closed
wants to merge 2 commits into from
Closed

[WIP] fix TPR single group, multiple versions issue #36157

wants to merge 2 commits into from

Conversation

adohe-zz
Copy link

@adohe-zz adohe-zz commented Nov 3, 2016 •
edited

Ref here for more detail. @deads2k @brendandburns ptal.

This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit 8223198. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Nov 3, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit 8223198. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

deads2k
deads2k reviewed Nov 4, 2016
View reviewed changes
pkg/master/thirdparty/thirdparty.go
}
}

for _, routeToeRemove := range routesToRemove {
Copy link
Contributor

Choose a reason for hiding this comment

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

typo. Though ToeRemoval sounds interesting..... :)

deads2k
deads2k reviewed Nov 4, 2016
View reviewed changes
pkg/registry/extensions/rest/thirdparty_controller.go
if len(group) == 0 {
return thirdpartyprefix
}
return thirdpartyprefix + "/" + group + "/" + resource
Copy link
Contributor

Choose a reason for hiding this comment

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

This path doesn't look right for a versioned API.

Copy link
Author

Choose a reason for hiding this comment

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

@deads2k yes, I am thinking about this PR together with the second issue: Multiple Versions - it's currently impossible to add more than one version for a group

@k8s-github-robot k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 11, 2016
@apelisse apelisse removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 11, 2016
@adohe-zz
Copy link
Author

Close this in favor of #36977

@adohe-zz adohe-zz closed this Nov 17, 2016
@adohe-zz adohe-zz deleted the third_party_resources branch November 17, 2016 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deads2k deads2k deads2k left review comments

Assignees

@smarterclayton smarterclayton

Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@adohe-zz @k8s-ci-robot @deads2k @smarterclayton @apelisse @googlebot @k8s-github-robot @adohe