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

ADO-2059 added Artifactory as a tool source #78

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 59 additions & 20 deletions src/config.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
use std::{collections::BTreeMap, env, fmt};

use semver::VersionReq;
use serde::{Deserialize, Serialize};

use crate::{
ci_string::CiString, error::ForemanError, fs, paths::ForemanPaths, tool_provider::Provider,
};
use semver::VersionReq;
use serde::{Deserialize, Serialize};
use std::{collections::BTreeMap, env, fmt};

#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
#[serde(untagged)]
Expand All @@ -20,50 +18,62 @@ pub enum ToolSpec {
gitlab: String,
version: VersionReq,
},
Artifactory {
artifactory: String,
path: String,
version: VersionReq,
},
}

impl ToolSpec {
pub fn cache_key(&self) -> CiString {
match self {
ToolSpec::Github { github, .. } => CiString(github.clone()),
ToolSpec::Gitlab { gitlab, .. } => CiString(format!("gitlab@{}", gitlab)),
ToolSpec::Artifactory { artifactory, .. } => CiString(artifactory.clone()),
}
}

pub fn source(&self) -> &str {
pub fn source(&self) -> String {
match self {
ToolSpec::Github { github: source, .. } | ToolSpec::Gitlab { gitlab: source, .. } => {
source
source.to_owned()
}
ToolSpec::Artifactory {
artifactory: source,
path,
..
} => {
format!("{}/{}", source, path) //might want to use Url crate to enforce propper paths
afujiwara-roblox marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

pub fn version(&self) -> &VersionReq {
match self {
ToolSpec::Github { version, .. } | ToolSpec::Gitlab { version, .. } => version,
ToolSpec::Github { version, .. }
| ToolSpec::Gitlab { version, .. }
| ToolSpec::Artifactory { version, .. } => version,
}
}

pub fn provider(&self) -> Provider {
match self {
ToolSpec::Github { .. } => Provider::Github,
ToolSpec::Gitlab { .. } => Provider::Gitlab,
ToolSpec::Artifactory { .. } => Provider::Artifactory,
}
}
}

impl fmt::Display for ToolSpec {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(
f,
"{}.com/{}@{}",
match self {
ToolSpec::Github { .. } => "github",
ToolSpec::Gitlab { .. } => "gitlab",
},
self.source(),
self.version(),
)
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)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, any rusty way to simplify it like before?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could inline the match like above, but this reads just fine to me

}
}

Expand Down Expand Up @@ -152,13 +162,21 @@ mod test {
}
}

fn new_gitlab<S: Into<String>>(github: S, version: VersionReq) -> ToolSpec {
fn new_gitlab<S: Into<String>>(gitlab: S, version: VersionReq) -> ToolSpec {
ToolSpec::Gitlab {
gitlab: github.into(),
gitlab: gitlab.into(),
version,
}
}

fn new_artifactory<S: Into<String>>(artifactory: S, path: S, version: VersionReq) -> ToolSpec {
ToolSpec::Artifactory {
artifactory: artifactory.into(),
path: path.into(),
version: version,
}
}

fn version(string: &str) -> VersionReq {
VersionReq::parse(string).unwrap()
}
Expand Down Expand Up @@ -189,6 +207,27 @@ mod test {
.unwrap();
assert_eq!(gitlab, new_gitlab("user/repo", version("0.1.0")));
}

#[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"),
)
);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any other tests we'd like to see?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}

#[test]
Expand Down
6 changes: 6 additions & 0 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ pub enum ForemanError {
ToolsNotDownloaded {
tools: Vec<String>,
},
Other {
message: String,
},
}

impl ForemanError {
Expand Down Expand Up @@ -308,6 +311,9 @@ impl fmt::Display for ForemanError {
Self::ToolsNotDownloaded { tools } => {
write!(f, "The following tools were not installed:\n{:#?}", tools)
}
Self::Other { message } => {
write!(f, "{}", message)
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/tool_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ impl ToolCache {
log::info!("Downloading {}", tool);

let provider = providers.get(&tool.provider());
let releases = provider.get_releases(tool.source())?;
let releases = provider.get_releases(tool.source().as_str())?;

// Filter down our set of releases to those that are valid versions and
// have release assets for our current platform.
Expand Down
36 changes: 36 additions & 0 deletions src/tool_provider/artifactory.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
//! Slice of GitHub's API that Foreman consumes.

use crate::{
error::{ForemanError, ForemanResult},
paths::ForemanPaths,
};

use super::{Release, ToolProviderImpl};

#[derive(Debug)]
#[allow(unused)]
pub struct ArtifactoryProvider {
paths: ForemanPaths,
}

impl ArtifactoryProvider {
pub fn new(paths: ForemanPaths) -> Self {
Self { paths }
}
}
#[allow(unused)]
impl ToolProviderImpl for ArtifactoryProvider {
fn get_releases(&self, repo: &str) -> ForemanResult<Vec<Release>> {
Err(ForemanError::Other {
message: "Artifactory is not yet supported. Please use Github or Gitlab as your source"
.to_owned(),
})
}

fn download_asset(&self, url: &str) -> ForemanResult<Vec<u8>> {
Err(ForemanError::Other {
message: "Artifactory is not yet supported. Please use Github or Gitlab as your source"
.to_owned(),
})
}
}
11 changes: 9 additions & 2 deletions src/tool_provider/mod.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
mod artifactory;
mod github;
mod gitlab;

use std::{collections::HashMap, fmt};

use crate::{error::ForemanResult, paths::ForemanPaths};
use artifactory::ArtifactoryProvider;
use github::GithubProvider;
use gitlab::GitlabProvider;

use crate::{error::ForemanResult, paths::ForemanPaths};

pub trait ToolProviderImpl: fmt::Debug {
fn get_releases(&self, repo: &str) -> ForemanResult<Vec<Release>>;

Expand All @@ -18,6 +19,7 @@ pub trait ToolProviderImpl: fmt::Debug {
pub enum Provider {
Github,
Gitlab,
Artifactory,
}

impl fmt::Display for Provider {
Expand All @@ -28,6 +30,7 @@ impl fmt::Display for Provider {
match self {
Provider::Github => "GitHub",
Provider::Gitlab => "GitLab",
Provider::Artifactory => "Artifactory",
}
)
}
Expand All @@ -49,6 +52,10 @@ impl ToolProvider {
Provider::Gitlab,
Box::new(GitlabProvider::new(paths.clone())),
);
providers.insert(
Provider::Artifactory,
Box::new(ArtifactoryProvider::new(paths.clone())),
);
Self { providers }
}

Expand Down