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: remove trailing slash from s3_uri #198

Merged
merged 2 commits into from
Nov 30, 2021
Merged

fix: remove trailing slash from s3_uri #198

merged 2 commits into from
Nov 30, 2021

Conversation

Bento007
Copy link
Contributor

Reviewers

Functional: @atolopko-czi

Readability: @seve


Remove trailing slash from the s3_uri that is used in the /s3_uri/<s3_uri>/api/v0.3 requests, to ensure that all requests are properly utilizing the CloudFront cache. This woull change this:

/cellxgene/s3_uri/s3%253A%252F%252Fhosted-cellxgene-dev%252Fpbmc3k.cxg%252F/api/v0.3/genesets

to this

/cellxgene/s3_uri/s3%253A%252F%252Fhosted-cellxgene-dev%252Fpbmc3k.cxg/api/v0.3/genesets

Changes

  • remove the trailing slash from S3_URIs before returning in GET */s3_uri

@Bento007 Bento007 added bug Something isn't working backend core backend (rest api, web, engine) labels Nov 29, 2021
@Bento007 Bento007 self-assigned this Nov 29, 2021
@codecov
Copy link

codecov bot commented Nov 29, 2021

Codecov Report

Merging #198 (8bb2304) into main (1aed6aa) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #198   +/-   ##
=======================================
  Coverage   70.39%   70.40%           
=======================================
  Files         132      132           
  Lines       10020    10022    +2     
=======================================
+ Hits         7054     7056    +2     
  Misses       2966     2966           
Flag Coverage Δ
frontend 70.40% <100.00%> (+<0.01%) ⬆️
javascript 70.40% <100.00%> (+<0.01%) ⬆️
smokeTest ?
unitTest 70.40% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
server/app/api/util.py 100.00% <100.00%> (ø)
app/api/util.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1aed6aa...8bb2304. Read the comment docs.

Copy link
Contributor

@atolopko-czi atolopko-czi left a comment

Choose a reason for hiding this comment

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

LGTM. Could be worth adding a test to help protect any inadvertent removal of this no-slash requirement. Similarly adding a comment here to explain the purpose would be useful.

@atolopko-czi
Copy link
Contributor

Beyond the scope of this PR, it seems worth "fixing" this in data portal as well by having an explicit, validated format there (i.e. no trailing slash). Similarly for explorer URLs, since there exists a hack to query for explorer URLs that may or may not have a trailing slash. I can create stories for this, but seeking input.

@Bento007
Copy link
Contributor Author

It seems like a low priority, but something worth doing eventually.

@Bento007 Bento007 changed the title remove trailing slash from s3_uri fix: remove trailing slash from s3_uri Nov 29, 2021
@Bento007 Bento007 enabled auto-merge (squash) November 29, 2021 23:39
@Bento007 Bento007 merged commit fdda2f4 into main Nov 30, 2021
@Bento007 Bento007 deleted the tsmith/cache-fix branch November 30, 2021 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend core backend (rest api, web, engine) bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants