Implement custom github: URL scheme in Go
#4134
Conversation
jandubois
changed the title
github: URL scheme in Go
|
I've turned this back into a Draft for now because I still want to make 2 changes compared to the old bash script:
Combined this will allow you to use |
This looks confusing to me. |
Users probably don't want this file name either, as the file name doesn't explain what this YAML is for. Maybe it should be resolved as |
That doesn't make sense to me, the Many projects use the same name for their org and main repo. We would be using
Yet another network roundtrip, and I doubt that
It is a direct extension of "an empty REPO means it is the same as the ORG". I don't see what useful purpose treating extra slashes as redundant would serve. It is not really different from
It creates an instance with the
How so? Why can
I would prefer not adding another heuristic. Let's keep things simple! I still think my suggestions are sensible! |
What I meant was that |
|
An alternative to |
Maybe I've not been clear enough: I expect It can be a YAML file as well, but I think it would almost always be a symlink. So in my example I assumed it would be a symlink to |
jandubois
force-pushed
the
url-github
branch
2 times, most recently
from
5ae31b5 to
4e3ca40
Compare
|
I've implemented the changes I suggested above. I also created a ❯ ls -la ~/git/jandubois
total 0
drwxr-xr-x@ 5 jan staff 160 Oct 3 22:10 .
drwxr-xr-x@ 11 jan staff 352 Oct 3 22:09 ..
drwxr-xr-x@ 12 jan staff 384 Oct 3 22:11 .git
lrwxr-xr-x@ 1 jan staff 19 Oct 3 22:10 .lima.yaml -> templates/demo.yaml
drwxr-xr-x@ 3 jan staff 96 Oct 3 22:09 templates
❯ l tmpl url github:jandubois
https://raw.githubusercontent.com/jandubois/jandubois/main/templates/demo.yamlThe symlink check is actually done for any file called @AkihiroSuda I don't think the tests should reference my personal repos; can I create The tests should probably move to BATS later, but there is a lot of plugin functionality that still needs tests. |
Why both? The dot file seems better for the specially reserved file name.
The unit test for resolving the URL doesn't need to rely on an actual repo? If you want to write an "integration" test that actually fetches a YAML content from a repo, it is fine to rely on jandubois/jandubois. |
So you can have Example: ❯ l tmpl url github:jandubois//docs/
https://raw.githubusercontent.com/jandubois/jandubois/main/templates/demo.yamlIt doesn't really matter because if the file is not a symlink, then it will not be treated specially: ❯ l tmpl url github:jandubois//test/lima.yaml
https://raw.githubusercontent.com/jandubois/jandubois/main/test/lima.yaml
I want to turn them into BATS anyways; I wrote them in Go because at first I forgot that they need network access. There is no reason they shouldn't be integration tests and test the real thing.
It would have to be Note that the I will for now continue to use |
|
There is one other change I want to make: the branch/tag/sha should follow the repo, not the path/filename: I think I originally put it last because it comes last in Additional benefits:
Not super important, but feels better to me. |
This may not work well when the tag contains a slash. |
|
Thanks, didn't think of that. So I won't change it. |
I changed my mind on this. It is simpler to say "when only a directory is specified, then the filename will be By using Thanks for questioning this; it was not a good idea to support |
This is a reimplementation of the bash code in lima-vm#3937 (comment) Signed-off-by: Jan Dubois <[email protected]>
|
I've rewritten the tests using BATS: ❯ ./lib/bats-core/bin/bats -T tests/url-github.bats
url-github.bats
✓ github:jandubois/jandubois/templates/demo.yaml@main [79]
✓ github:jandubois/jandubois/templates/demo.yaml [465]
✓ github:jandubois/jandubois/templates/demo [414]
✓ github:jandubois/jandubois/.lima.yaml [566]
✓ github:jandubois/jandubois/@v0.0.0 [206]
✓ github:jandubois/jandubois [414]
✓ github:jandubois//templates/demo.yaml@main [40]
✓ github:jandubois//templates/demo.yaml [358]
✓ github:jandubois//templates/demo [391]
✓ github:jandubois//.lima.yaml [419]
✓ github:jandubois//@v0.0.0 [112]
✓ github:jandubois// [457]
✓ github:jandubois/ [409]
✓ github:[email protected] [103]
✓ github:jandubois [408]
✓ github:jandubois/jandubois/docs/.lima.yaml@main [222]
✓ github:jandubois/jandubois/docs/.lima.yaml [434]
✓ github:jandubois/jandubois/docs/.lima [652]
✓ github:jandubois/jandubois/docs/ [466]
✓ github:jandubois//docs/[email protected] [209]
✓ github:jandubois//docs/.lima.yaml [441]
✓ github:jandubois//docs/.lima [545]
✓ github:jandubois//docs/@v0.0.0 [109]
✓ github:jandubois//docs/ [428]
✓ .lima.yaml is retained when it is not a symlink [530]
✓ hidden files without an extension get a .yaml extension [398]
✓ files that have an extension do not get a .yaml extension [357]
✓ github: URLs are EXPERIMENTAL [445]
✓ Empty github: url returns an error [49]
✓ Missing org returns an error [47]
30 tests, 0 failures in 12 secondsI've also simplified the rules if a file looks like a symlink: Must be non-empty and have no newline, space, or colon. That's it. I'm somewhat surprised to see that the API call to determine the default branch adds 300-400ms to the processing time. The symlink resolution only takes an additional 50-100ms. |
Out of curiosity I've sorted the individual tests into the 4 different buckets: No network requestsSymlink resolution via githubusercontentDefault branch lookup via GitHub APIBoth symlink and branch requests |
|
I got one more idea today: We could check if the This would allow you to have That way you don't need to keep the template in the ORG/ORG repo, which may be unwanted.
(see next comment) |
Here is an example using our own Assume we have Then Because: (1) this is an ORG repo (same name as org), (2) the filename is If this fails, or if the content of this file is anything but a But since it is a Which means I don't like the complexity of the rules, and that it relies on the requested tag not existing in the ORG repo. But this assumption is probably true in almost every case (where you want to redirect to another repo), and it improves the usability of the Because of the potential for confusion (refetching from default branch when the requested tag doesn't exist) I would limit the tag propagation to ORG repos. I'm just discussing the idea here; it should still be a separate PR if we decide that we want to implement it. |
This is probably fine, but we have to confirm whether we need some sort of "CORS" restriction; do we need to disallow a redirect to a different org? |
IDK. It feels inconsistent because we don't restrict redirects on http/https template URLs to the same domain: limactl create https://example.com/templates/example.yamlThis can redirect wherever it likes, and we would still load the template. However, this feature is only meant to be used to redirect from the ORG repo to a different repo within the same ORG, so it feels fine to me to restrict this, at least initially. But this means also that we only allow It is always better to only allow a subset at first, and expand later based on feedback, instead of providing it all, and having to take some of it away again later. Anyways, I hope this PR can be approved as-is, and this feature can be a separate PR. |
There was a problem hiding this comment.
Thanks
We need a documentation, probably as a subpage of https://lima-vm.io/docs/templates/
(https://lima-vm.io/docs/templates/github)
| github:jandubois// | ||
| github:jandubois/ | ||
| github:[email protected] | ||
| github:jandubois |
There was a problem hiding this comment.
No need to cover in this PR, but maybe we can even further shorten this form to gh:jandubois
There was a problem hiding this comment.
We could shorten it, but template: is kind of long too, so github: fits right in, and maybe gh: is a bit obscure for some people.
Or did you want gh: as an alias, and not as a replacement? Not sure if that is good, as people might wonder what the difference would be.
There was a problem hiding this comment.
As an alias. No need to cover in this PR anyway.
Yes, not just for |
There was a problem hiding this comment.
Is it possible to add unit tests for functions in the file?
There was a problem hiding this comment.
I originally had unit tests, but replaced them with the BATS (integration) tests because some of them require network access.
I don't see any benefit to splitting the tests up, and find the BATS tests more concise anyways.
This is a reimplementation of the bash code in #3937 (comment)