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

refactor: remove nested orgs labels route #19104

Merged
merged 1 commit into from
Jul 30, 2020

Conversation

AlirieGray
Copy link
Contributor

@AlirieGray AlirieGray commented Jul 27, 2020

This PR removes an outdated swagger definition for the orgs/orgID/labels route which was not being used, and removes the label handler from the org handler to reflect this change.

Continuation of #19102

@AlirieGray AlirieGray force-pushed the add-org-case-to-embedded-labels branch from b87bfa8 to 4167205 Compare July 27, 2020 22:53
@AlirieGray AlirieGray requested a review from lyondhill July 27, 2020 22:54
@AlirieGray AlirieGray force-pushed the add-org-case-to-embedded-labels branch from 4167205 to 68b06b6 Compare July 28, 2020 16:34
@AlirieGray AlirieGray requested a review from a team as a code owner July 28, 2020 16:34
@AlirieGray AlirieGray requested review from ebb-tide and removed request for a team July 28, 2020 16:34
@AlirieGray AlirieGray force-pushed the add-org-case-to-embedded-labels branch from 68b06b6 to 1fc5cb5 Compare July 28, 2020 16:38
@AlirieGray AlirieGray changed the title refactor: add a lookup by orgID to labels handler refactor: remove nested orgs labels route Jul 28, 2020
@AlirieGray AlirieGray force-pushed the add-org-case-to-embedded-labels branch 2 times, most recently from b4171fc to 2345008 Compare July 28, 2020 16:44
glinton
glinton previously approved these changes Jul 28, 2020
http/swagger.yml Show resolved Hide resolved
@glinton glinton dismissed their stale review July 28, 2020 16:58

more of a comment

@AlirieGray AlirieGray force-pushed the add-org-case-to-embedded-labels branch 2 times, most recently from a914cea to e07c090 Compare July 28, 2020 18:40
@AlirieGray AlirieGray force-pushed the add-org-case-to-embedded-labels branch 3 times, most recently from c13657e to 6a40dd4 Compare July 29, 2020 16:49
@AlirieGray AlirieGray requested a review from 121watts July 29, 2020 17:24
Copy link
Contributor

@glinton glinton left a comment

Choose a reason for hiding this comment

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

code looks fine, i'm hesitant to approve as i don't have context regarding the snippet you posted in your other pr. Maybe @ebb-tide would be able to confidently say whether this would have any ill effects or not (tests seem to pass so i'd say ✔️, but they've missed things before)

This code is not behind a feature flag, as the route that this label handler was supporting is not functional currently and is not being used in our UIs. Instead of returning an empty array of labels, this route will now return a 404.

@AlirieGray AlirieGray force-pushed the add-org-case-to-embedded-labels branch from 6a40dd4 to 5b2d33f Compare July 30, 2020 16:30
@AlirieGray AlirieGray merged commit d1b8e98 into master Jul 30, 2020
@AlirieGray AlirieGray deleted the add-org-case-to-embedded-labels branch July 30, 2020 18:40
@stephanie-engel
Copy link
Contributor

stephanie-engel commented Aug 11, 2020

@AlirieGray @russorat
I noticed that the nested org labels route was removed from OSS, but it still exists (and works) in Cloud 2. Is that to be expected, or should there be another PR to remove that code from Cloud 2

Nvm, I was using an older Cloud 2 cluster that didn't have the latest changes.

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.

5 participants