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

fix: host URL for instances in different regions #4374

Merged
merged 1 commit into from
Jan 27, 2023

Conversation

PeterSchafer
Copy link
Collaborator

What does this PR do?

Previously we assumed that the root hostname shouldn't start with "app." but this is actually the consistent endpoint for cross region usage.

@PeterSchafer PeterSchafer force-pushed the fix/license_urls branch 3 times, most recently from 4bda823 to 1b8a5f7 Compare January 26, 2023 17:35
@PeterSchafer PeterSchafer marked this pull request as ready for review January 26, 2023 17:35
@PeterSchafer PeterSchafer requested review from a team as code owners January 26, 2023 17:35
// given an api url that starts with api means, that we can replace "api" by "app".

const apiUrl = new URL(apiUrlString);
apiUrl.host = apiUrl.host.replace(/^api\./, 'app.');
Copy link
Contributor

Choose a reason for hiding this comment

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

what about https://snyk.io/api? That should become https://app.xx.snyk.io/api, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess, what I want to say is that you don't always have an api subdomain ;).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Absolutely correct, we might not have "api" sub-domains which is fine, they remain unaltered. As far as I understood, https://snyk.io/api is the legacy, still supported url.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After discussing with @bastiandoetsch offline, we decided to reduce the impact of the change a bit, by replacing "api." by an empty string only, not replacing by "app.". Which is basically what was there before just that we keep Urls that have "app." already unaltered.

@PeterSchafer PeterSchafer merged commit cebc353 into master Jan 27, 2023
@PeterSchafer PeterSchafer deleted the fix/license_urls branch January 27, 2023 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants