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

Conversation

@smacfarlane
Copy link
Contributor

commented Aug 12, 2019

Closes #6762

This adds the ability for the cli to disable the creation of a build group when uploading packages. This is useful when building sets of package outside of builder, such as we do with a base-plans refresh.

Because builder tests presence of the builder flag to determine if a build group should be created, I've chosen to represent this as an Option, rather than a boolean. By representing it as an Option, it felt more in-line with what we end up passing to Builder.

Behavior change

Previously, if any dependencies were uploaded along with the target artifact they would each trigger a build group (the Thundering Herd problem). Now, dependencies will always disable job creation.

Signed-off-by: Scott Macfarlane smacfarlane@chef.io

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>
@chef-expeditor

This comment has been minimized.

Copy link

commented Aug 12, 2019

Hello smacfarlane! Thanks for the pull request!

Here is what will happen next:

  1. Your PR will be reviewed by the maintainers.
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

@@ -822,6 +822,14 @@ 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");

// Builder checks for presence of the 'builder' param to determine
// if a job group should be created.
let disable_build_group = if m.is_present("NO_BUILD") {

This comment has been minimized.

Copy link
@chefsalim

chefsalim Aug 12, 2019

Member

It might be simpler/less code to just pass and use this as a bool like force_upload

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 12, 2019

Contributor

Shouldn't this be

Suggested change
let disable_build_group = if m.is_present("NO_BUILD") {
let disable_build_group = if m.is_present("DISABLE_BUILD") {

to correspond to the declaration in cli.rs?

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 12, 2019

Contributor

I agree with @chefsalim. I'd prefer this as its own type rather than an Option<&str>. I know we may need to convert to a string when encoding the URL to send to the builder API, but that can be done at that stage. Here, I think it makes things more confusing and unnecessarily couples the hab crate to implementation details of builder.

My preference would be for adding a new type to https://github.com/habitat-sh/habitat/blob/master/components/builder-api-client/src/lib.rs like:

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

or

#[derive(Copy, Clone, Debug)]
pub struct BuildOnUpload(pub bool);

This achieves some additional type checking, makes the coupling looser, and makes the caller:

        upload_into_depot(…, None, …)

into the clearer

        upload_into_depot(…, BuildOnUpload::Disable, …)

or

        upload_into_depot(…, BuildOnUpload(false), …)
components/hab/src/cli.rs Outdated Show resolved Hide resolved
@@ -822,6 +822,14 @@ 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");

// Builder checks for presence of the 'builder' param to determine
// if a job group should be created.
let disable_build_group = if m.is_present("NO_BUILD") {

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 12, 2019

Contributor

Shouldn't this be

Suggested change
let disable_build_group = if m.is_present("NO_BUILD") {
let disable_build_group = if m.is_present("DISABLE_BUILD") {

to correspond to the declaration in cli.rs?

@@ -447,6 +447,7 @@ impl BuilderAPIProvider for ArtifactoryClient {
pa: &mut PackageArchive,
token: &str,
_force_upload: bool,
_disable_build_group: Option<&str>,

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 12, 2019

Contributor

It seems like we should reject user input that includes unsupported options. Otherwise, this leads to a rather confusing UX. Perhaps pulling some of the functionality out of BuilderAPIProvider, so that it only covers what all implementations support would lead to a better-fitting abstraction.

This comment has been minimized.

Copy link
@chefsalim

chefsalim Aug 12, 2019

Member

I wouldn't optimize for the provider model as that is likely going away soon (since we;re not going the Artifactory plug in direction any longer)

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 12, 2019

Contributor

Good to know! Any reason not to take the Artifactory code out right away?

This comment has been minimized.

Copy link
@chefsalim

chefsalim Aug 14, 2019

Member

The only reason to not take it out right away is to avoid complicating things in case we have to revert any of the other PRs that also touch that code (paranoia).

@@ -822,6 +822,14 @@ 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");

// Builder checks for presence of the 'builder' param to determine
// if a job group should be created.
let disable_build_group = if m.is_present("NO_BUILD") {

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 12, 2019

Contributor

I agree with @chefsalim. I'd prefer this as its own type rather than an Option<&str>. I know we may need to convert to a string when encoding the URL to send to the builder API, but that can be done at that stage. Here, I think it makes things more confusing and unnecessarily couples the hab crate to implementation details of builder.

My preference would be for adding a new type to https://github.com/habitat-sh/habitat/blob/master/components/builder-api-client/src/lib.rs like:

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

or

#[derive(Copy, Clone, Debug)]
pub struct BuildOnUpload(pub bool);

This achieves some additional type checking, makes the coupling looser, and makes the caller:

        upload_into_depot(…, None, …)

into the clearer

        upload_into_depot(…, BuildOnUpload::Disable, …)

or

        upload_into_depot(…, BuildOnUpload(false), …)

if let Some(val) = disable_build_group {
url.query_pairs_mut().append_pair("builder", val);
}

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 12, 2019

Contributor

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.

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 13, 2019

Contributor

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?

This comment has been minimized.

Copy link
@chefsalim

chefsalim Aug 14, 2019

Member

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.

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 14, 2019

Contributor

That makes more sense; thanks for the context.

@@ -526,6 +526,8 @@ 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 DISABLE_BUILD: --("disable-build") "Do not create a build group on package upload. This flag has no \

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 12, 2019

Contributor

I think boolean-type flags conform to the --foo/--no-foo pattern enhances consistency and usability, making this --no-build (which is also a bit shorter).

smacfarlane added 2 commits Aug 13, 2019
…ption

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>
[rustfmt]
Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>
@smacfarlane

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

@baumanj (GH not letting me respond directly on your feedback 😞 ) I like the idea of an enum rather than a boolean to describe what that parameter is. When I started to go down that road for implementation though, in some cases it started to read awkwardly: disable_build = BuildOnUpload::Enable.

I thought about changing the language from disable_build to auto_build, but that would be misleading when reading the code as that parameter can't enable builds, it can only disable them.

I've gone with a simple boolean for now, what are your thoughts?

Copy link
Contributor

left a comment

Since this is new functionality, it would be nice to add some sort of test

@baumanj

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

I like the idea of an enum rather than a boolean to describe what that parameter is. When I started to go down that road for implementation though, in some cases it started to read awkwardly: disable_build = BuildOnUpload::Enable.

Can you show me what you mean? With the enum option, I think your code would look like:

if let BuildOnUpload::Disable = build_on_upload {
    url.query_pairs_mut().append_pair("builder", "true");
}

or

match build_on_upload {
    BuildOnUpload::Enable => {},
    BuildOnUpload::Disable => url.query_pairs_mut().append_pair("builder", "true"),
}

Neither of those look particularly awkward to me. Though maybe it's a question of being used to if let statements and how they use pattern matching. It's also possible to use a regular if:

if build_on_upload == BuildOnUpload::Disable {
    url.query_pairs_mut().append_pair("builder", "true");
}

Though, you'd have to derive PartialEq for the BuildOnUpload enum, and I'd consider it somewhat less idiomatic rust.

If you dislike those approaches, I think using pub struct BuildOnUpload(pub bool) is just as good as the enum approach. That gives:

if let BuildOnUpload(false) = build_on_upload {
    url.query_pairs_mut().append_pair("builder", "true");
}

I thought about changing the language from disable_build to auto_build, but that would be misleading when reading the code as that parameter can't enable builds, it can only disable them.

That could be addressed by changing the enum to

pub enum BuildOnUpload {
    PackageDefault,
    Disable,
}

Possibly with a doc comment explaining things in more detail.

I've gone with a simple boolean for now, what are your thoughts?

It's definitely better than the Option<&str> approach, and I'm not going to stand in your way if you're happy with it. I'd still prefer the readability and additional safety of having a specific type, though.

smacfarlane added 2 commits Aug 14, 2019
Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>
Rustfmt
Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>
@smacfarlane

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

pub enum BuildOnUpload {
    PackageDefault,
    Disable,
}

@baumanj That makes a lot more sense and have reworked it to use this pattern. I was previously fixated on it coming from a boolean and being an enable/disable toggle. Thanks for this advice 😺

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>
Copy link
Member

left a comment

Looks good to me

@smacfarlane

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

In lieu of code tests at this time, I did a quick behavior test and verified if a job group was created in the builder UI and added #6857 to track the need for tests:

Default behavior:

env RUST_LOG=debug cargo run --bin hab -- pkg upload ~/src/smacfarlane/habitat-empty-test-plans/results/smacfarlane-empty-plan-0.0.3-20190815204152-x86_64-linux.hart
...
[2019-08-15T21:04:59Z DEBUG habitat_http_client::api_client] POST https://bldr.habitat.sh/v1/depot/pkgs/smacfarlane/empty-plan/0.0.3/20190815204152?checksum=1421a9f398f8b014c83a0680fd6c6153e57a926d8be4f7329b29b33ff9577179&target=x86_64-linux&forced=false with ApiClient { endpoint: "https://bldr.habitat.sh/v1", inner: Client }

With --no-build:

env RUST_LOG=debug cargo run --bin hab -- pkg upload --no-build ~/src/smacfarlane/habitat-empty-test-plans/results/smacfarlane-empty-plan-0.0.3-20190815210934-x86_64-linux.hart
...
[2019-08-15T21:10:00Z DEBUG habitat_http_client::api_client] POST https://bldr.habitat.sh/v1/depot/pkgs/smacfarlane/empty-plan/0.0.3/20190815210934?checksum=8055508fc9ae5fdd839e6aa2c38214e4d8e63719160d4547858ab43de0733ee6&target=x86_64-linux&forced=false&builder=true with ApiClient { endpoint: "https://bldr.habitat.sh/v1", inner: Client }
@smacfarlane smacfarlane merged commit 9ccde2a into master Aug 19, 2019
5 checks passed
5 checks passed
DCO This commit has a DCO Signed-off-by
Details
buildkite/habitat-sh-habitat-master-verify Build #3083 passed (21 minutes, 31 seconds)
Details
buildkite/habitat-sh-habitat-master-website Build #211 passed (56 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details
@chef-ci chef-ci deleted the sm/add-disable-build-flag branch Aug 19, 2019
@mwrock mwrock added the X-feature label Aug 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.