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

Ensure artifact base url has a trailing slash #442

Closed
wants to merge 1 commit into from

Conversation

soapy1
Copy link
Contributor

@soapy1 soapy1 commented Nov 22, 2024

Fixes conda-incubator/conda-store#995

Description

Base url's should have a trailing slash to denote that they are not a relative reference. When there is no trailing slash, using the URL constructor will cause the last part of the path to be cut off.

So, a base url my.server.com/conda-store gets parsed different from my.server.com/conda-store/

ref: https://developer.mozilla.org/en-US/docs/Web/API/URL_API/Resolving_relative_references

So, as an example in src/features/artifacts/components/ArtifactsItem.tsx for a Log artifact

const baseUrl = "http://localhost:8080/conda-store/"
const route = new URL(artifact.route, baseUrl).toString();
// route is correctly `localhost:8080/conda-store/api/v1/build/<build number>/log`

const baseUrl = "http://localhost:8080/conda-store"
const route = new URL(artifact.route, baseUrl).toString();
// route is incorrectly `localhost:8080/api/v1/build/<build number>/log`

Pull request checklist

  • Did you test this change locally?
  • Did you update the documentation (if required)?
  • Did you add/update relevant tests for this change (if required)?

Base url's should have a trailing slash to denote that they are
not a relative reference. When there is no trailing slash, using
the URL constructor will cause the last part of the path to be
cut off.

So, a base url my.server.com/conda-store gets parsed different
from my.server.com/conda-store/

ref: https://developer.mozilla.org/en-US/docs/Web/API/URL_API/Resolving_relative_references
@@ -15,8 +15,8 @@ const isPathAbsolute = (path: string) => {

export const artifactBaseUrl = (apiUrl: string, baseUrl: string) => {
if (isPathAbsolute(apiUrl)) {
return apiUrl;
return `${apiUrl}/`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It there a better more typescript-y way of doing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure where the appropriate place to write a test is, would appreciate some direction 🙏

@gabalafou
Copy link
Contributor

Hey, thanks for putting up a fix for this.

There's an issue that if somebody on the backend later configures the base url to have a trailing slash, then this PR will cause the base url to have two slashes at the end, which may or may not cause a problem for the server, but I would prefer to keep things tidy and not potentially end up with URLs in the future that have double slashes, so I opened a PR that I think fixes the code in a more robust way (#443).

Would you mind giving that one a review while I close this PR?

@gabalafou gabalafou closed this Nov 22, 2024
@soapy1 soapy1 deleted the artifact-base-url branch November 22, 2024 19:35
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.

[BUG] - getting artifacts from the ui returns not found error
2 participants