-
Notifications
You must be signed in to change notification settings - Fork 69
feat: add support for relative URLs #2305
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
Conversation
|
Thanks! Commenting on the PR description so far, not the code yet: My reason for wanting to retire RFC 3986: Uniform Resource Identifier (URI): Generic Syntax We should tell the users: we just follow the standard (and ideally use a library that resolves relative URIs for us so we don't mess up and end up with a slightly incompatible wheel)
A search for rust relative uri resolution points me to |
I checked that crate and it does not support URLs like this: |
|
Another top hit from that search is https://docs.rs/fluent-uri/latest/fluent_uri/ which seems to target our use case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I think there are still some loose ends. In particular the case of the missing context.
|
|
||
| match &self { | ||
| FileSource::Text { content } => write!(file, "{}", &content)?, | ||
| FileSource::Remote { url } => Transfer::get(&url.to_string(), &mut file)?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks OK but then I realize that url can be a relative UriRef in which case the user will get an error.
IMHO you forgot this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not forgot, but I simply let Transfer to handle the error. In the future I would like to replace url::Url with fluent_uri::UriRef and have better handling here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment about it.
Co-authored-by: Martin Vidner <[email protected]>
mvidner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is good, but seriously we are missing documentation.
Looking at the "Solution" part of the PR description, it seems obvious to me that the minimal docs change should be the JSON schema. Please do :)
|
As mentioned in IRC, I decided to change the implementation a bit. The logic to resolve the URLs is now invoked from |
mvidner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The help for agama download needs adjusting
| let absolute_url = if uri.has_scheme() { | ||
| uri.to_string() | ||
| } else { | ||
| uri.resolve_against(&context.source)?.to_string() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation! Again we are adding a secret feature. Please locate the 2 occurrences of relurl in Agama source, both are affected by this PR, one by this commit :)
Co-authored-by: Martin Vidner <[email protected]>
mvidner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now, thank you!
Prepare to release Agama 15: * #2258 * #2270 * #2277 * #2279 * #2283 * #2284 * #2285 * #2286 * #2287 * #2288 * #2291 * #2292 * #2293 * #2295 * #2297 * #2299 * #2300 * #2301 * #2302 * #2303 * #2305 * #2306 * #2307 * #2308 * #2309 * #2313 * #2314 * #2315 * #2317 * #2318 * #2319 * #2320 * #2321 * #2322 * #2323 * #2324 * #2325 * #2328 * #2329 * #2330 * #2331 * #2335 * #2336 * #2337 * #2338 * #2339 * #2340 * #2342 * #2345 * #2346 * #2348 * #2349 * #2350 * #2351 * #2352 * #2353 * #2354 * #2355 * #2357 * #2358 * #2359 * #2360 * #2361 * #2362 * #2363 * #2364 * #2365 * #2366 * #2368 * #2369 * #2370 * #2371 * #2372 * #2374 * #2377 * #2378 * #2379 * #2380 * #2381 * #2382 * #2384 * #2385 * #2386 * #2388 * #2389 * #2390 * #2391 * #2392 * #2394 * #2397 * #2398 * #2401 * #2403
Mention relative references, as replacement for relurl Implemented in agama-project/agama#2305 , and Leo M from QA found that this piece of docs was not updated for that. Rendered at: https://agama-project.github.io/docs/user/urls Looking at that page, I also found a rendering problem at the bottom, fixing that too.
Problem
The
relurlschema is used in several places, especially in AutoYaST profiles. It can be used to refer a script, a file, repositories, etc. They are useful because you do not need to specify the full URL, but it uses a URL which is relative to the profile URL.The
relurlis not directly supported by the FileFromUrl API, but it is implemented in different places. The goal of this PR is adding support to read files and scripts from relative locations.relurlis an AutoYaST-specific invention.Solution
Instead of porting
relurl, introduce the concept of URL reference, well known from HTML<a href=...>and standardized in RFC3986 (Uniform Resource Identifier (URI): Generic Syntax).A URL reference is either a URL (absolute), or a relative reference (typically just a path), resolved in relation to a base URL.
{ files: [ { destination: '/etc/issue.d/agama.issue', url: 'issue-sles', }, { destination: '/etc/issue.d/readme.issue', url: 'http://192.168.122.1/agama/issue-readme', }, ], scripts: { post: [ { name: "disable-nginx", url: "disable-nginx.sh", }, { name: "disable-firewall", url: "http://192.168.122.1/agama/disable-firewall.sh", }, ], } }A new Url
The Url struct from the url crate does not support relative URLs. This PR uses fluent_uri but the old
Urlhas not been replaced everywhere yet.What is the base URL?
The first problem when using relative URLs is to determine which is the base URL. In this case, it is the profile's URL. We could store the URL somewhere (in the manager?) but the true is that it can change with every call to
agama profile import. And what aboutagama config load?For this first implementation, I have decided to inject the URL depending on the context:
agama profile import: it uses the URL of the profile.agama config load: it injects the current directory. It will not work as expected when working remotely, but it should do when callingagama config loadlocally. Another option might be to just report an error about not being able to resolve the URL.(Upcoming changes: according to #2289,
agama config generate BASE_URL(the replacement for the most complex part ofagama profile import) will be responsible for resolving any relative references, according to the explicit base given to it, and replace them with absolute URL in its JSON output.agama config loadwill not need to care.)Other changes
To do
resolve_urlmodify the receiver?).Testing