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 api url setting has trailing slash #997

Closed
wants to merge 1 commit into from

Conversation

soapy1
Copy link
Contributor

@soapy1 soapy1 commented Nov 21, 2024

Fixes #995

Description

This is required for the "log and artifact" links in the ui to work.

For example, follow the instructions in #995 to reproduce the issue. With this change, the links provided in the Logs and Artifacts section correctly point to /conda-store/api/v1/<rest of the api path>.

Pull request checklist

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

This is required for the "log and artifact" links in the ui to
work.
Copy link

netlify bot commented Nov 21, 2024

Deploy Preview for conda-store canceled.

Name Link
🔨 Latest commit f95db45
🔍 Latest deploy log https://app.netlify.com/sites/conda-store/deploys/673fb7a23ca5780008995272

@@ -22,7 +22,7 @@
REACT_APP_AUTH_TOKEN: "",
REACT_APP_STYLE_TYPE: "green-accent",
REACT_APP_SHOW_LOGIN_ICON: "true",
REACT_APP_API_URL: "{{ url_for('redirect_root_to_ui') }}",
REACT_APP_API_URL: "{{ url_for('redirect_root_to_ui') }}/",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if there is a better way to do this, like maybe on the ui side? The rest of the interactions between the ui and api seem to work properly with and without this change.

@peytondmurray
Copy link
Contributor

Looks like this is not being tested, otherwise we would have caught this. Would it be possible to write a test to hit these logs/artifacts endpoints with and without trailing slashes?

@soapy1 soapy1 added needs: discussion 💬 This item needs team-level discussion before scoping needs: review 👀 needs: testing ✅ labels Nov 22, 2024
@soapy1
Copy link
Contributor Author

soapy1 commented Nov 22, 2024

Would it be possible to write a test to hit these logs/artifacts endpoints with and without trailing slashes?

Thinking about this issue more, I think it would be better for this issue to be fixed in conda-store-ui opposed to conda-store.

The crux of the issue is that the logs/artifacts in the ui are links to some api endpoint. Without this slash the url_prefix is ignored for building these links. But, the rest of the api interactions between conda-store and conda-store-ui work without this trailing slash. So, that makes me think that there is an issue with how the ui is generating the links.

I took a quick look at the ui code, but I don't really understand how it works so I could be really off base here. @peytondmurray @gabalafou if you have some time could you take a look?

@soapy1
Copy link
Contributor Author

soapy1 commented Nov 22, 2024

ok so, I think this fixes things on the ui side conda-incubator/conda-store-ui#442 and this PR can be closed.

@gabalafou
Copy link
Contributor

I think that the front-end app should be robust enough to handle whether an implementer supplies the base URL with or without the trailing slash

Hope you don't mind me closing this pull request in favor of conda-incubator/conda-store-ui#443.

@gabalafou gabalafou closed this Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done 💪🏾
Development

Successfully merging this pull request may close these issues.

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