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

ref(hybrid-cloud): fix monitor urls using organization_slug #42825

Merged
merged 2 commits into from
Jan 5, 2023

Conversation

cathteng
Copy link
Member

@cathteng cathteng commented Jan 5, 2023

The format of the url should be /api/0/organization/{organization_slug}/monitors/...

For HC-512, HC-513, HC-514

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jan 5, 2023
@cathteng cathteng requested review from RyanSkonnord and a team January 5, 2023 19:30
@cathteng cathteng marked this pull request as ready for review January 5, 2023 19:30
@cathteng cathteng requested a review from a team as a code owner January 5, 2023 19:30
@JoshFerge
Copy link
Member

will this break the current monitors API? and would our docs and cli have to be updated?

@JoshFerge JoshFerge requested a review from a team January 5, 2023 19:36
@cathteng
Copy link
Member Author

cathteng commented Jan 5, 2023

@JoshFerge i had previously changed the urls and then changed what was being used on the frontend without a problem -- this is like a second pass to correct what i had missed the first time (changing the slugs #42300 #42356 #42384, changing the frontend #42728)

docs and cli may need to be updated though

@JoshFerge JoshFerge requested a review from davidenwang January 5, 2023 19:41
@JoshFerge
Copy link
Member

@cathyteng17 kk. will defer to @davidenwang

Copy link
Contributor

@davidenwang davidenwang left a comment

Choose a reason for hiding this comment

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

thanks for changing these

),
),
url(
r"^organizations/(?P<organization_slug>[^\/]+)/monitors/",
Copy link
Contributor

Choose a reason for hiding this comment

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

placement of these seems fine for now since its close to the original monitors urls, but could you add a note here that when we delete the old monitors routes that we should probably include these routes into the larger /organizations/ route tree below?

@cathteng cathteng merged commit 91cf672 into master Jan 5, 2023
@cathteng cathteng deleted the cathy/hc/fix-monitor-urls-with-orgslug branch January 5, 2023 23:14
@github-actions github-actions bot locked and limited conversation to collaborators Jan 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants