suppress autobuild in builder api

[pozsgaic]

Code to suppress autobuild using via cli command "hab config apply"

[mwrock]

I think this should be [] since we would not want to suppress any origins by default.

[pozsgaic]

Sure thing

[mwrock]

per comment above, we don't want test to be the default.

[mwrock]

deserialize_into_vec will transform origins to uppercase but actual origins can be mixed case.

[pozsgaic]

Will add and use a similar function like deserialize_into_vec.
IMO deserialize_into_vec should have something in its name to indicate that it is changing the case.
Like:
deserialize_into_vec_upper

[mwrock]

I don't think you need a deserializer. I think a Vec<String> should work as is. Like bootstrap_nodes in

builder/components/builder-events/src/kafka.rs

Line 40 in 5298517

pub bootstrap_nodes: Vec<String>,

[pozsgaic]

OK, but does that change how user will enter these using "hab config apply"?

After making the change, the test case in config.rs fails here:
let config = Config::from_raw(content).unwrap();

The test line:
suppress_autobuild_origins = "origin1, origin2"
Fails to load unless I change it to ["origin1", "origin2"], which would be awkward if the user
has to enter this at the command line.

[mwrock]

right it would need to be a proper toml array. That format is in line with other config we have. If you wanted to stick with "origin1, origin2" I think you would need to create a deserializer.

[pozsgaic]

OK - I think we should stick to whatever convention we are already using.

[mwrock]

should we just keep this inline since it is just a one-liner and not used anywhere else?

[mwrock]

this is really just testing contains applied to a vector so Its probably not gaining us much coverage. However, I think you could make a small refactoring to build_plans to factor out the db call and allow for the creation of a dummy Project and get much better unit test coverage of this method.

[pozsgaic]

I am not clear on how we can create a dummy Project and pass it in to build_plans. If we passed a Project in, then we are now building a project, not the plans. We would move the plans iterator out of this function, then it would become build_plan.
Please clarify.

[mwrock]

ahh right. That iterator complicates this. Well you could still extract more logic into something like:

fn should_reject_build(plan: &PlanWithTarget, project: Project, config: &Config, repo_url: &str) -> bool {
    if repo_url != project.vcs_data {
        warn!("Repo URL ({}) doesn't match project vcs data ({}). Aborting.",
              repo_url, project.vcs_data);
        return true;
    }

    if !config.api.build_targets.contains(&plan.1) {
        debug!("Rejecting build with target: {:?}", plan.1);
        return true;
    }

    if config.api.suppress_autobuild_origins.contains(&plan.0.origin.to_string()) {
        debug!("Skipping autobuild for origin {:?}", &plan.0.origin);
        return true;
    }
    return false
}

And here would be an example "happy path" test:

    #[test]
    fn valid_plan_is_not_rejected() {
        let mut config = Config::default();
        config.api.suppress_autobuild_origins = vec!["hest".to_string(), "sest".to_string()];

        let project = Project { id: 1,
            owner_id: 1,
            origin: "origin".to_string(),
            package_name: "pkg".to_string(),
            name: "origin/pkg".to_string(),
            plan_path: "plan.sh".to_string(),
            target: "x86_64-linux".to_string(),
            vcs_type: "git".to_string(),
            vcs_data: "https://github.com/habitat-sh/testapp.git".to_string(),
            vcs_installation_id: None,
            auto_build: true,
            created_at: None,
            updated_at: None };

        let plan = PlanWithTarget(Plan { name: "pkg".to_string(),
            origin: "origin".to_string(),
            version: None},
            PackageTarget::from_str("x86_64-linux").unwrap());

        assert_eq!(should_reject_build(&plan, project, &config, "https://github.com/habitat-sh/testapp.git"), false);
    }

I'd leave it up to you if you wanted to add something like that. I'm just not sure if the test as it stands in the PR is worth including.

[pozsgaic]

Yeah this makes sense. We can encapsulate the failure and test each separately. I think it is good to have coverage for all of these so I'll go ahead and add tests for the repo_url and build_targets check.

[mwrock]

I would not include the top two shell scripts in this PR. I can see how it would help you as you build this but we already have end to end tests around hab config apply. See the first couple test cases in https://github.com/habitat-sh/habitat/tree/main/test/end-to-end/multi-supervisor/testcases.

[mwrock]

you might extract create_project and a create_plan functions that return a mutable project and a mutable plan and also have them take origin, package, target, and repo_url args. You could then reuse them in all the tests.

[pozsgaic]

Thanks for pointing me to the hab config apply testing. I still would like to see that this succeeds inside of builder-api by observing whether or not the build kicks off depending on the current origin set defined in suppress_autobuild_origins.