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

Add ability to disable job group creation from the cli #6843

Merged
merged 6 commits into from Aug 19, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions components/builder-api-client/src/artifactory.rs
Expand Up @@ -10,6 +10,7 @@ use crate::{error::{Error,
hab_http::ApiClient,
response::ResponseExt,
BoxedClient,
BuildOnUpload,
BuilderAPIProvider,
DisplayProgress,
OriginKeyIdent,
Expand Down Expand Up @@ -447,6 +448,7 @@ impl BuilderAPIProvider for ArtifactoryClient {
pa: &mut PackageArchive,
token: &str,
_force_upload: bool,
_auto_build: BuildOnUpload,
progress: Option<Self::Progress>)
-> Result<()> {
let checksum = pa.checksum()?;
Expand Down
8 changes: 8 additions & 0 deletions components/builder-api-client/src/builder.rs
Expand Up @@ -11,6 +11,7 @@ use crate::{error::{Error,
response::{err_from_response,
ResponseExt},
BoxedClient,
BuildOnUpload,
BuilderAPIProvider,
DisplayProgress,
OriginKeyIdent,
Expand Down Expand Up @@ -807,6 +808,7 @@ impl BuilderAPIProvider for BuilderAPIClient {
pa: &mut PackageArchive,
token: &str,
force_upload: bool,
auto_build: BuildOnUpload,
progress: Option<Self::Progress>)
-> Result<()> {
let checksum = pa.checksum()?;
Expand All @@ -828,6 +830,12 @@ impl BuilderAPIProvider for BuilderAPIClient {
.append_pair("checksum", &checksum)
.append_pair("target", &target.to_string())
.append_pair("forced", &force_upload.to_string());

// Builder uses presence of the `builder` param to disable builds.
// Only send the parameter when we the user requests builds be disabled.
if let BuildOnUpload::Disable = auto_build {
url.query_pairs_mut().append_pair("builder", "true");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just an interesting rust FYI that Option and Result implement IntoIterator, which would allow you to do this as:

            url.query_pairs_mut()
               .extend_pairs(disable_build_group.map(|v| ("builder", v)));

Per my other comments, I think we should use a non-Option type here, but I thought this was handy to know.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be clear, if there's a builder=XXX item in the query parameters, then that disables the auto-build functionality? That's really unexpected. Why is the builder API like that?

Copy link
Contributor

Choose a reason for hiding this comment

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

When Builder does an upload it wants to disable the auto build (since it is doing those builds, and we don't want an infinite loop). The query param naming is historical.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes more sense; thanks for the context.

};
debug!("Reading from {}", &pa.path.display());

Expand Down
7 changes: 7 additions & 0 deletions components/builder-api-client/src/lib.rs
Expand Up @@ -240,6 +240,12 @@ pub struct ReverseDependencies {
pub rdeps: Vec<String>,
}

#[derive(Clone, Copy, Debug)]
pub enum BuildOnUpload {
PackageDefault,
Disable,
}

pub trait BuilderAPIProvider: Sync + Send {
type Progress;

Expand Down Expand Up @@ -291,6 +297,7 @@ pub trait BuilderAPIProvider: Sync + Send {
pa: &mut PackageArchive,
token: &str,
force_upload: bool,
auto_build: BuildOnUpload,
progress: Option<Self::Progress>)
-> Result<()>;

Expand Down
1 change: 0 additions & 1 deletion components/butterfly/src/rumor.rs
Expand Up @@ -471,7 +471,6 @@ mod storage {
rs.increment_update_counter();
assert_eq!(rs.get_update_counter(), 1);
}

}
}

Expand Down
1 change: 1 addition & 0 deletions components/hab/src/cli.rs
Expand Up @@ -526,6 +526,7 @@ pub fn get(feature_flags: FeatureFlag) -> App<'static, 'static> {
(@arg FORCE: --force "Skips checking availability of package and \
force uploads, potentially overwriting a stored copy of a package. \
(default: false)")
(@arg NO_BUILD: --("no-build") "Disable auto-build for all packages in this upload.")
(@arg HART_FILE: +required +multiple {file_exists}
"One or more filepaths to a Habitat Artifact \
(ex: /home/acme-redis-3.0.7-21120102031201-x86_64-linux.hart)")
Expand Down
59 changes: 35 additions & 24 deletions components/hab/src/command/pkg/upload.rs
Expand Up @@ -16,6 +16,7 @@

use crate::{api_client::{self,
BoxedClient,
BuildOnUpload,
Client},
common::{command::package::install::{RETRIES,
RETRY_WAIT},
Expand Down Expand Up @@ -46,12 +47,14 @@ use std::path::{Path,
/// * Fails if it cannot find a package
/// * Fails if the package doesn't have a `.hart` file in the cache
/// * Fails if it cannot upload the file
#[allow(clippy::too_many_arguments)]
pub fn start(ui: &mut UI,
bldr_url: &str,
additional_release_channel: &Option<ChannelIdent>,
token: &str,
archive_path: &Path,
force_upload: bool,
auto_build: BuildOnUpload,
key_path: &Path)
-> Result<()> {
let mut archive = PackageArchive::new(PathBuf::from(archive_path));
Expand Down Expand Up @@ -108,6 +111,7 @@ pub fn start(ui: &mut UI,
(&ident, target),
additional_release_channel,
force_upload,
auto_build,
&mut archive)
};
match retry(delay::Fixed::from(RETRY_WAIT).take(RETRIES), upload) {
Expand All @@ -130,39 +134,45 @@ pub fn start(ui: &mut UI,
/// automatically put into the `unstable` channel, but if
/// `additional_release_channel` is provided, packages will be
/// promoted to that channel as well.
#[allow(clippy::too_many_arguments)]
fn upload_into_depot(ui: &mut UI,
api_client: &BoxedClient,
token: &str,
(ident, target): (&PackageIdent, PackageTarget),
additional_release_channel: &Option<ChannelIdent>,
force_upload: bool,
auto_build: BuildOnUpload,
mut archive: &mut PackageArchive)
-> Result<()> {
ui.status(Status::Uploading, archive.path.display())?;
let package_uploaded =
match api_client.put_package(&mut archive, token, force_upload, ui.progress()) {
Ok(_) => true,
Err(api_client::Error::APIError(StatusCode::CONFLICT, _)) => {
println!("Package already exists on remote; skipping.");
true
}
Err(api_client::Error::APIError(StatusCode::UNPROCESSABLE_ENTITY, _)) => {
return Err(Error::PackageArchiveMalformed(format!("{}",
archive.path
.display())));
}
Err(api_client::Error::APIError(StatusCode::NOT_IMPLEMENTED, _)) => {
println!("Package platform or architecture not supported by the targeted depot; \
skipping.");
false
}
Err(api_client::Error::APIError(StatusCode::FAILED_DEPENDENCY, _)) => {
ui.fatal("Package upload introduces a circular dependency - please check \
pkg_deps; skipping.")?;
false
}
Err(e) => return Err(Error::from(e)),
};
let package_uploaded = match api_client.put_package(&mut archive,
token,
force_upload,
auto_build,
ui.progress())
{
Ok(_) => true,
Err(api_client::Error::APIError(StatusCode::CONFLICT, _)) => {
println!("Package already exists on remote; skipping.");
true
}
Err(api_client::Error::APIError(StatusCode::UNPROCESSABLE_ENTITY, _)) => {
return Err(Error::PackageArchiveMalformed(format!("{}",
archive.path
.display())));
}
Err(api_client::Error::APIError(StatusCode::NOT_IMPLEMENTED, _)) => {
println!("Package platform or architecture not supported by the targeted depot; \
skipping.");
false
}
Err(api_client::Error::APIError(StatusCode::FAILED_DEPENDENCY, _)) => {
ui.fatal("Package upload introduces a circular dependency - please check pkg_deps; \
skipping.")?;
false
}
Err(e) => return Err(Error::from(e)),
};
ui.status(Status::Uploaded, ident)?;

// Promote to additional_release_channel if specified
Expand Down Expand Up @@ -208,6 +218,7 @@ fn attempt_upload_dep(ui: &mut UI,
(ident, target),
additional_release_channel,
false,
BuildOnUpload::Disable,
&mut archive)
} else {
let archive_name = ident.archive_name_with_target(target).unwrap();
Expand Down
8 changes: 8 additions & 0 deletions components/hab/src/main.rs
Expand Up @@ -27,6 +27,7 @@ use hab::{cli::{self,
ORIGIN_ENVVAR,
PRODUCT,
VERSION};
use habitat_api_client::BuildOnUpload;
use habitat_common::{self as common,
cli::{cache_key_path_from_matches,
FS_ROOT},
Expand Down Expand Up @@ -822,6 +823,12 @@ fn sub_pkg_upload(ui: &mut UI, m: &ArgMatches<'_>) -> Result<()> {
// before allowing a write to the backend, this bypasses the check
let force_upload = m.is_present("FORCE");

let auto_build = if m.is_present("NO_BUILD") {
BuildOnUpload::Disable
} else {
BuildOnUpload::PackageDefault
};

let token = auth_token_param_or_env(&m)?;
let artifact_paths = m.values_of("HART_FILE").unwrap(); // Required via clap
for artifact_path in artifact_paths.map(Path::new) {
Expand All @@ -831,6 +838,7 @@ fn sub_pkg_upload(ui: &mut UI, m: &ArgMatches<'_>) -> Result<()> {
&token,
artifact_path,
force_upload,
auto_build,
&key_path)?;
}
Ok(())
Expand Down
Binary file added components/studio/libexec/busybox
Binary file not shown.
Binary file added components/studio/libexec/hab
Binary file not shown.