ADO-2059 added Artifactory as a tool source #78
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
src/config.rs
Outdated
| let res = match self { | ||
| ToolSpec::Github { .. } => format!("github.com/{}@{}", self.source(), self.version()), | ||
| ToolSpec::Gitlab { .. } => format!("gitlab.com/{}@{}", self.source(), self.version()), | ||
| ToolSpec::Artifactory { .. } => format!("{}@{}", self.source(), self.version()), | ||
| }; | ||
| write!(f, "{}", res) |
There was a problem hiding this comment.
Again, any rusty way to simplify it like before?
There was a problem hiding this comment.
I think you could inline the match like above, but this reads just fine to me
src/config.rs
Outdated
| #[test] | ||
| fn artifactory_from_artifactory_field() { | ||
| let artifactory: ToolSpec = toml::from_str( | ||
| &[ | ||
| r#"artifactory = "https://artifactory-edge1.rbx.com""#, | ||
| r#"path = "generic-rbx-local-tools/rotriever/""#, | ||
| r#"version = "0.5.4""#, | ||
| ] | ||
| .join("\n"), | ||
| ) | ||
| .unwrap(); | ||
| assert_eq!( | ||
| artifactory, | ||
| new_artifactory( | ||
| "https://artifactory-edge1.rbx.com", | ||
| "generic-rbx-local-tools/rotriever/", | ||
| version("0.5.4"), | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Any other tests we'd like to see?
There was a problem hiding this comment.
Do we want to support leading and trailing backslashes in the artifactory and path sections such as
[tools.rotrieve]
artifactory = "https://artifactory-edge1.rbx.com/"
path = "/generic-rbx-local-tools/rotriever/"
version = "0.5.4"
There was a problem hiding this comment.
I'm not really sure about this, it'd be nice to be flexible. I suspect when you join them together it doesn't matter too much if there's an extra slash, but it might also not matter at all if we use a Url class that has its own notions of "path" and "domain" and probably handles the rest for us?
afujiwara-roblox
changed the title
src/config.rs
Outdated
| source.to_owned() | ||
| } | ||
| ToolSpec::Artifactory { | ||
| artifactory: source, | ||
| path, | ||
| .. | ||
| } => { | ||
| format!("{}/{}", source, path) |
There was a problem hiding this comment.
Any rusty way to avoid making a new String? Maybe add a new field and move line 47 to when we initialize the ToolSpec::Artifactory?
There was a problem hiding this comment.
Hmm, I see your point; our definition of source gets more complicated with the artifactory definition, I almost wonder if we could separate source and path in the abstraction in general?
That way you'd have sources like: https://github.com, https://gitlab.com, https://somehost.artifactory.com, https://otherhost.artifactory.com, etc.
And then paths like: Roblox/foreman, path/to/artifact, etc.
There was a problem hiding this comment.
One way we could be thinking about this would be to have a separate definition that aliases artifactory servers to sources, and a toml that looks roughly like this:
[sources]
# implicit default sources are:
# github = "https://github.com"
# gitlab = "https://gitlab.com"
rbx_artifactory = "https://rbx.artifactory.com"
[tools]
# this tool is still on github
tarmac = { github = "Roblox/tarmac", version = "0.7.0" }
# this tool has been migrated
rotrieve = { rbx_artifactory = "generic-rbx-sage-infra/tools/rotriever", version = "0.5.12" }There was a problem hiding this comment.
An alternative would be to have every tool spec be composed of source (more like host) + path + version, in which case we'd maybe want the existing stuff like rotrieve = { github = "roblox/rotriever", version = "0.5.12" } is just a shorthand for:
[tools.rotrieve]
source = "https://github.com"
path = "roblox/rotriever"
version = "0.5.12"
src/config.rs
Outdated
| let res = match self { | ||
| ToolSpec::Github { .. } => format!("github.com/{}@{}", self.source(), self.version()), | ||
| ToolSpec::Gitlab { .. } => format!("gitlab.com/{}@{}", self.source(), self.version()), | ||
| ToolSpec::Artifactory { .. } => format!("{}@{}", self.source(), self.version()), | ||
| }; | ||
| write!(f, "{}", res) |
There was a problem hiding this comment.
I think you could inline the match like above, but this reads just fine to me
src/config.rs
Outdated
| source.to_owned() | ||
| } | ||
| ToolSpec::Artifactory { | ||
| artifactory: source, | ||
| path, | ||
| .. | ||
| } => { | ||
| format!("{}/{}", source, path) |
There was a problem hiding this comment.
Hmm, I see your point; our definition of source gets more complicated with the artifactory definition, I almost wonder if we could separate source and path in the abstraction in general?
That way you'd have sources like: https://github.com, https://gitlab.com, https://somehost.artifactory.com, https://otherhost.artifactory.com, etc.
And then paths like: Roblox/foreman, path/to/artifact, etc.
src/config.rs
Outdated
| source.to_owned() | ||
| } | ||
| ToolSpec::Artifactory { | ||
| artifactory: source, | ||
| path, | ||
| .. | ||
| } => { | ||
| format!("{}/{}", source, path) |
There was a problem hiding this comment.
One way we could be thinking about this would be to have a separate definition that aliases artifactory servers to sources, and a toml that looks roughly like this:
[sources]
# implicit default sources are:
# github = "https://github.com"
# gitlab = "https://gitlab.com"
rbx_artifactory = "https://rbx.artifactory.com"
[tools]
# this tool is still on github
tarmac = { github = "Roblox/tarmac", version = "0.7.0" }
# this tool has been migrated
rotrieve = { rbx_artifactory = "generic-rbx-sage-infra/tools/rotriever", version = "0.5.12" }
src/config.rs
Outdated
| source.to_owned() | ||
| } | ||
| ToolSpec::Artifactory { | ||
| artifactory: source, | ||
| path, | ||
| .. | ||
| } => { | ||
| format!("{}/{}", source, path) |
There was a problem hiding this comment.
An alternative would be to have every tool spec be composed of source (more like host) + path + version, in which case we'd maybe want the existing stuff like rotrieve = { github = "roblox/rotriever", version = "0.5.12" } is just a shorthand for:
[tools.rotrieve]
source = "https://github.com"
path = "roblox/rotriever"
version = "0.5.12"| unable to parse Foreman configuration file (at {{CWD}}foreman.toml): data is not properly formatted for tool: | ||
|
|
||
| [tools.tool] | ||
| invalid = "roblox/tooling" | ||
| version = "0.0.0" | ||
|
|
There was a problem hiding this comment.
I had to change the snapshot. We lose line information since we're longer relying on Serde to deserialize the toml. I figured replacing the incorrect line with the incorrect tool is good enough
There was a problem hiding this comment.
Yeah, that's probably reasonable, especially given all the cases in which Serde's line annotation falls apart
|
Closing to break up into smaller PRs
|
added Artifactory as a tool source