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

Immutable URIs WIP #1935

Merged
merged 6 commits into from
Dec 4, 2020
Merged

Immutable URIs WIP #1935

merged 6 commits into from
Dec 4, 2020

Conversation

nl0
Copy link
Member

@nl0 nl0 commented Nov 25, 2020

Description

Add /uri/* route to resolve Quilt package URIs as per spec

TODO

@codecov-io
Copy link

codecov-io commented Nov 26, 2020

Codecov Report

Merging #1935 (88d7c5d) into master (3c87327) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1935   +/-   ##
=======================================
  Coverage   90.40%   90.40%           
=======================================
  Files          64       64           
  Lines        7792     7792           
=======================================
  Hits         7044     7044           
  Misses        748      748           
Flag Coverage Δ
api-python 89.35% <ø> (ø)
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 3c87327...88d7c5d. Read the comment docs.

@nl0 nl0 force-pushed the immutable-uris branch 2 times, most recently from 5868ba8 to 254e933 Compare November 30, 2020 07:43
@nl0 nl0 requested review from fiskus and akarve November 30, 2020 14:07
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.

First shallow reading is done

catalog/app/constants/routes.js Outdated Show resolved Hide resolved
catalog/app/containers/UriResolver/UriResolver.js Outdated Show resolved Hide resolved
catalog/app/containers/UriResolver/UriResolver.js Outdated Show resolved Hide resolved
catalog/app/utils/PackageUri.js Show resolved Hide resolved
catalog/app/utils/PackageUri.js Show resolved Hide resolved
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.

It cost me some brain cycles to realize where can I copy the package's URI. Even I saw it this morning. But I understand this was "quick implementation vs perfect UX" balanced. Maybe we can steal GitHub UI and show copy button near hash.

Screenshot from 2020-11-30 18-42-59

Also, I was expecting, that click on the "resolve" button will redirect to the resolved URL.


Possible problem: as a user I don't know how to close/hide "Resolved from URI" notification. As a developer, I can see, that some users can bookmark URL with query string or send it to a colleague and will be stuck with this notification.
We can redirect to the canonical URL automatically, or (and) add a close button to notification


Issues beyond the task's scope (or not, I don't know): errors are not helpful.

https:///b/fiskus-sandbox-dev/packages/fiskus/2020-11-1?resolvedFrom=quilt%2Bs3%3A%2F%2Ffiskus-sandbox-dev%23package%3Dfiskus%2F2020-11-1
Error: wrong revision, actually: wrong package

http://localhost:3000/b/fiskus-sandbox-dev/packages/fiskus/?resolvedFrom=quilt%2Bs3%3A%2F%2Ffiskus-sandbox-dev%23package%3Dfiskus%2F
Error: nothing here, do you need to log in, and exceptions in console. Actually, no such package, I'm logged in

catalog/app/utils/PackageUri.spec.js Outdated Show resolved Hide resolved
@nl0
Copy link
Member Author

nl0 commented Dec 3, 2020

It cost me some brain cycles to realize where can I copy the package's URI. Even I saw it this morning. But I understand this was "quick implementation vs perfect UX" balanced. Maybe we can steal GitHub UI and show copy button near hash.

gh ux doesnt fit here exactly, bc a button near a hash is expected to be related to that hash, not to the "canonical URI"

Also, I was expecting, that click on the "resolve" button will redirect to the resolved URL.

i think i'll implement this and ditch the package box

Possible problem: as a user I don't know how to close/hide "Resolved from URI" notification. As a developer, I can see, that some users can bookmark URL with query string or send it to a colleague and will be stuck with this notification.
We can redirect to the canonical URL automatically, or (and) add a close button to notification

i'll add a close btn

Issues beyond the task's scope (or not, I don't know): errors are not helpful.

https:///b/fiskus-sandbox-dev/packages/fiskus/2020-11-1?resolvedFrom=quilt%2Bs3%3A%2F%2Ffiskus-sandbox-dev%23package%3Dfiskus%2F2020-11-1
Error: wrong revision, actually: wrong package

well, we may want to add some check to see if the package actually exists (there's at least one revision of that package), but it seems out of scope, you're right

http://localhost:3000/b/fiskus-sandbox-dev/packages/fiskus/?resolvedFrom=quilt%2Bs3%3A%2F%2Ffiskus-sandbox-dev%23package%3Dfiskus%2F
Error: nothing here, do you need to log in, and exceptions in console. Actually, no such package, I'm logged in

it's a 404, bc the url doesnt match any route, which seems more or less correct

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.

Consider null exception

catalog/app/constants/routes.js Show resolved Hide resolved
catalog/app/containers/Bucket/PackageTree.js Show resolved Hide resolved
catalog/app/utils/PackageUri.spec.js Show resolved Hide resolved
catalog/app/utils/PackageUri.spec.js Show resolved Hide resolved
catalog/app/utils/PackageUri.js Outdated Show resolved Hide resolved
@nl0 nl0 merged commit 7e9d55e into master Dec 4, 2020
@nl0 nl0 deleted the immutable-uris branch December 4, 2020 08:53
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.

3 participants