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

Retire use of api-legacy endpoints #3296

Merged
merged 1 commit into from
Jul 7, 2020
Merged

Retire use of api-legacy endpoints #3296

merged 1 commit into from
Jul 7, 2020

Conversation

n1zea144
Copy link
Contributor

Retires the use of the api-legacy endpoint. Should be deployed in coordination with backend PR #7653.

@n1zea144 n1zea144 added the chore This PR is related to code maintenance label Jun 25, 2020
@n1zea144 n1zea144 mentioned this pull request Jun 25, 2020
Merged
kalletlak
kalletlak reviewed Jun 25, 2020
View reviewed changes
Copy link
Member

@kalletlak kalletlak left a comment

Choose a reason for hiding this comment

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

Looks like api-legacy is used even in MatchMinerAPI.ts

zhx828
zhx828 requested changes Jun 26, 2020
View reviewed changes
Copy link
Member

@zhx828 zhx828 left a comment

Choose a reason for hiding this comment

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

@n1zea144 thanks for the pr.

  • I'm a bit confused the new proxy api route. Shouldn't it be /api/proxy?
  • Like Karthik mentioned, could you modify the MatchMinerAPI.ts as well?
    Thanks.

src/shared/api/urls.ts Outdated
@@ -242,9 +242,9 @@ export function getSessionUrl() {
// TODO: remove this after switch to AWS. This is a hack to use proxy
// session-service from non apiRoot. We'll have to come up with a better
// solution for auth portals
return buildCBioPortalPageUrl('api-legacy/proxy/session');
return buildCBioPortalPageUrl('proxy/session');
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be api/proxy/session?

Copy link
Contributor Author

@n1zea144 n1zea144 Jun 26, 2020 •
edited

Choose a reason for hiding this comment

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

@zhx828 Maybe you are correct - I thought the buildCBioPortalPageUrl() handles adding the api root. Does it not (I'm not proficient in ts)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wait, I just realized I was confusing buildCBioPortalPageUrl with buildCBioPortalAPIUrl. They seem to have different behavior yet both calls pass different arguments. In any event, I'll update to app the "api/" back.

@n1zea144 n1zea144 force-pushed the retire-use-of-api-legacy-endpoints branch from 4018cb7 to 8d6649c Compare June 26, 2020 22:28
@n1zea144
Copy link
Contributor Author

@kalletlak @zhx828 I've added MatchMinerAPI.ts updates to this PR.

@n1zea144 n1zea144 force-pushed the retire-use-of-api-legacy-endpoints branch from 8d6649c to d6971e8 Compare June 26, 2020 22:37
@n1zea144 n1zea144 force-pushed the retire-use-of-api-legacy-endpoints branch from d6971e8 to a9b2074 Compare June 27, 2020 12:15
kalletlak
kalletlak approved these changes Jun 30, 2020
View reviewed changes
Copy link
Member

@kalletlak kalletlak left a comment

Choose a reason for hiding this comment

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

LGTM. Looks like prettier is failing, could you try to update code/changes by running prettier command locally

@zhx828 zhx828 requested a review from inodb June 30, 2020 14:03
zhx828
zhx828 approved these changes Jun 30, 2020
View reviewed changes
Copy link
Member

@zhx828 zhx828 left a comment

Choose a reason for hiding this comment

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

👍

@zhx828
Copy link
Member

zhx828 commented Jun 30, 2020

@inodb @alisman this pr cannot be merged until the backend is deployed. Do you think we should include it in the upcoming release?

@n1zea144 n1zea144 requested a review from kalletlak June 30, 2020 14:35
@alisman
Copy link
Collaborator

alisman commented Jun 30, 2020

@n1zea144 looks like a lot of e2e failures probably related. gonna run again and can take a look if it turns out to be true

@inodb
Copy link
Member

inodb commented Jul 1, 2020

@zhx828 i excluded it from this release from now, but can include in the next one

@n1zea144
Copy link
Contributor Author

n1zea144 commented Jul 1, 2020

Yeah, no rush to get this into this release - thanks everyone!

@inodb inodb merged commit a25c2b5 into cBioPortal:master Jul 7, 2020
@inodb inodb removed the DO NOT MERGE label Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhx828 zhx828 zhx828 approved these changes

@alisman alisman Awaiting requested review from alisman

@inodb inodb Awaiting requested review from inodb

@kalletlak kalletlak Awaiting requested review from kalletlak

Assignees
No one assigned
Labels
chore This PR is related to code maintenance
Projects
None yet
Milestone
No milestone
5 participants
@n1zea144 @zhx828 @alisman @inodb @kalletlak