APT-543 Download From Artifactory Support #82
Conversation
src/tool_provider/artifactory.rs
Outdated
| // file.uri will look something like /<version>/<artifact-name>, so uri will be ["", <version>, <artifact-name] | ||
| uri.next(); | ||
| let version = uri.next().unwrap(); | ||
| let asset_name = uri.next().unwrap(); |
There was a problem hiding this comment.
Should we do some validation here? For names, I don't think they have to be especially precise, but we could certainly validate that version is a valid semver version, right?
There was a problem hiding this comment.
I guess we probably do this elsewhere, now that I think about it. But I do wonder if we could detect if the target repo isn't structured the way we expect.
There was a problem hiding this comment.
Good point. I think it's worth at least ensuring that the repo path is structured the way we expect. If the path isn't what we expect, we should probably skip the file instead of panicking though.
There was a problem hiding this comment.
Foreman already does this version validation here https://github.com/Roblox/foreman/blob/main/src/tool_cache.rs#L115
| fn get_releases(&self, repo: &str, host: &Url) -> ForemanResult<Vec<Release>> { | ||
| let client = Client::new(); | ||
|
|
||
| let url = format!("{}artifactory/api/storage/{}", host, repo); |
There was a problem hiding this comment.
Does host always automatically have a trailing /? Or must it be provided somewhere?
There was a problem hiding this comment.
Speaking of, could we potentially use Url methods to do this in a slightly more structured way? https://docs.rs/url/latest/url/struct.Url.html#method.join
Or would it not provide any meaningful benefit?
There was a problem hiding this comment.
host does automatically have the trailing /, but using something like url join is probably worth it.
src/tool_provider/artifactory.rs
Outdated
| tag_name: version.to_string(), | ||
| assets, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Not a big deal, but I think this could be expressed a little more elegantly with a functional approach.
This PR adds Artifactory download support for Foreman