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

Hash-based package routes #1938

Merged
merged 3 commits into from
Nov 30, 2020
Merged

Hash-based package routes #1938

merged 3 commits into from
Nov 30, 2020

Conversation

nl0
Copy link
Member

@nl0 nl0 commented Nov 26, 2020

Description

Use top_hash in package routes, completely remove timestamps from the UI (timestamp revisions are still resolved, so it's backwards compatible).

TODO

@nl0 nl0 requested review from fiskus and akarve November 26, 2020 14:50
@nl0 nl0 mentioned this pull request Nov 26, 2020
4 tasks
@codecov-io
Copy link

codecov-io commented Nov 26, 2020

Codecov Report

Merging #1938 (8d1a671) into master (4b5c6db) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1938   +/-   ##
=======================================
  Coverage   90.36%   90.36%           
=======================================
  Files          64       64           
  Lines        7764     7764           
=======================================
  Hits         7016     7016           
  Misses        748      748           
Flag Coverage Δ
api-python 89.29% <ø> (ø)
lambda 92.40% <ø> (ø)

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


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 4b5c6db...8d1a671. Read the comment docs.

Copy link
Member

@fiskus fiskus left a comment

Choose a reason for hiding this comment

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

I will do manual tests tomorrow

catalog/app/utils/search.js Show resolved Hide resolved
@fiskus
Copy link
Member

fiskus commented Nov 27, 2020

Old URL works /b/fiskus-sandbox-dev/packages/fiskus/2011-11-16/tree/1605546363/
I don't know if this technically doable, we can make 301 redirect to new URL b/fiskus-sandbox-dev/packages/fiskus/2011-11-16/tree/67c766d5108b4dc2a7c3b74c8fd9287d880829158603de77bebf73288a7fdc81/

Also, I recall that we talked about short hashes (a la GitHub d5a76a3), but I don't remember what we decide

@nl0
Copy link
Member Author

nl0 commented Nov 27, 2020

I don't know if this technically doable, we can make 301 redirect to new URL

not possible until we implement server-side rendering, tho we can redirect to the more "canonical" hash-based route on the front-end.

@akarve do we want to redirect to the new routes?

Also, I recall that we talked about short hashes (a la GitHub d5a76a3), but I don't remember what we decide

this will require backend support, so i think we should add this feature later (along with tags support)

Copy link
Member

@fiskus fiskus left a comment

Choose a reason for hiding this comment

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

Didn't spot any bugs, except for inconsistency with old timestamps (not a bug, intentional)

Screenshot from 2020-11-27 09-51-44

Copy link
Member

@akarve akarve left a comment

Choose a reason for hiding this comment

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

Mostly works in testing. See a few notes in Slack.

@@ -13,6 +13,7 @@
## CLI

## Catalog, Lambdas
* [Changed] `top_hash`-based package routes (backwards compatible) ([#1938](https://github.com/quiltdata/quilt/pull/1938))
Copy link
Member

Choose a reason for hiding this comment

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

This is reasonable behavior but I'm wondering if forwarding to a dedicated /h/ or whatever hash route is more correct in terms of traceability? Please make a note that /timestamp URLs are not forwarded in any way, but that the user remains at /timestamp.

Copy link
Member

Choose a reason for hiding this comment

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

To be clear I am totally cool with current behavior (honor both types of routes, no redirect). I am just asking if anything becomes cleaner or safer with two routes.

@akarve akarve self-requested a review November 30, 2020 03:42
Copy link
Member

@akarve akarve left a comment

Choose a reason for hiding this comment

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

See potential bugs in Slack?

@akarve akarve self-requested a review November 30, 2020 03:52
@nl0 nl0 merged commit be9ff5b into master Nov 30, 2020
@nl0 nl0 deleted the hash-based-pkg-routing branch November 30, 2020 05:59
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.

4 participants