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 mac hab build to release process #6802

Merged
merged 6 commits into from Aug 13, 2019

Conversation

@scotthain
Copy link
Contributor

commented Aug 7, 2019

Still needs commit cleanup, rebase, and final verification.

@chef-expeditor

This comment has been minimized.

Copy link

commented Aug 7, 2019

Hello scotthain! 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!

@scotthain scotthain force-pushed the shain/iso_mac branch from 75d62f4 to d4718f7 Aug 7, 2019
Copy link
Contributor

left a comment

@scotthain Looking great so far! Couple thoughts/suggestions. Looking forward to seeing this go in!

.expeditor/scripts/build_mac_component.sh Outdated Show resolved Hide resolved
.expeditor/scripts/build_mac_component.sh Outdated Show resolved Hide resolved
@scotthain scotthain force-pushed the shain/iso_mac branch from 7e1e36b to fcfca44 Aug 7, 2019
@scotthain scotthain marked this pull request as ready for review Aug 7, 2019
@scotthain scotthain force-pushed the shain/iso_mac branch from fcfca44 to fac8e56 Aug 7, 2019
Copy link
Contributor

left a comment

I have some changes, but I'm excited to see this land 👍

.expeditor/scripts/build_mac_component.sh Outdated Show resolved Hide resolved
.expeditor/scripts/build_mac_component.sh Outdated Show resolved Hide resolved
.expeditor/scripts/build_mac_component.sh Outdated Show resolved Hide resolved
.expeditor/scripts/build_mac_component.sh Outdated Show resolved Hide resolved
.expeditor/scripts/build_mac_component.sh Outdated Show resolved Hide resolved
.expeditor/scripts/build_mac_component.sh Outdated Show resolved Hide resolved
.expeditor/scripts/build_mac_component.sh Outdated Show resolved Hide resolved
.expeditor/scripts/shared.sh Outdated Show resolved Hide resolved
.expeditor/scripts/shared.sh Outdated Show resolved Hide resolved
.expeditor/scripts/shared.sh Outdated Show resolved Hide resolved
@scotthain scotthain force-pushed the shain/iso_mac branch from b246516 to 6071b27 Aug 8, 2019
@christophermaier

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

The changes thus far look great! 👍

@@ -20,6 +20,7 @@ channel=$(get_release_channel)

echo "--- Channel: $channel - bldr url: $HAB_BLDR_URL"

declare -g hab_binary

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 12, 2019

Contributor

Might be nice to wrap this in a helper function and call that instead, just to help centralize the logic, and make it easier to evolve over time (I'd love it if we could get away from a global variable if possible... but I realize this is bash, after all 😦 )

@@ -0,0 +1,176 @@
#!/bin/bash

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 12, 2019

Contributor

This is just a random thought... would it be worth prefixing the names of functions in this (and similar) shared "library files" with the "namespace" the function comes from, as an additional clue to readers where code is defined and how it is intended to be used?

e.g., release.import_keys, release.get_latest_pkg_version, etc?

I'd know exactly where the function was defined, and if I'm using a release.* function in one of my end-to-end tests, I know I've done something very wrong.

@scotthain scotthain force-pushed the shain/iso_mac branch from 6f224a1 to a99f7c7 Aug 12, 2019
scotthain added 3 commits Aug 6, 2019
Signed-off-by: Scott Hain <shain@chef.io>
Signed-off-by: Scott Hain <shain@chef.io>
@scotthain scotthain force-pushed the shain/iso_mac branch from a99f7c7 to 857637a Aug 12, 2019
Signed-off-by: Scott Hain <shain@chef.io>
.expeditor/scripts/shared.sh Outdated Show resolved Hide resolved
Signed-off-by: Scott Hain <shain@chef.io>
@scotthain scotthain force-pushed the shain/iso_mac branch from b2a09f2 to 1abbbe9 Aug 12, 2019
Signed-off-by: Scott Hain <shain@chef.io>
echo "-- Hab and studio versions match! Found hab: ${hab_version:-null} - studio: ${studio_version:-null}. Upgrading :awesome:"
channel=$(get_release_channel)
${hab_binary:?} pkg install --binlink --force --channel "${channel}" core/hab
${hab_binary:?} pkg install --binlink --force --channel "${channel}" core/hab-studio

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 13, 2019

Contributor

Are these binlinks really needed? The whole idea of using $hab_binary is to unambiguously refer to a specific version of hab, and binlinking is just going to set the global hab to that specific version.

I also don't think there's a need to link to anything in core/hab-studio, since we don't call it directly, but rely the hab binary to resolve it correctly.

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 13, 2019

Contributor

I think it would probably be more readable and correct to do install using e.g. core/hab/${hab_version} and core/hab-studio/${studio_version}.

studio_version=$(get_latest_pkg_version_in_channel "hab-studio")

if [[ -n $hab_version && -n $studio_version && $hab_version == "$studio_version" ]]; then
echo "-- Hab and studio versions match! Found hab: ${hab_version:-null} - studio: ${studio_version:-null}. Upgrading :awesome:"

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 13, 2019

Contributor

If we get down here, $hab_version and $studio_version should always be set, so I don't think we need the :-null bits.

# workaround for https://github.com/habitat-sh/habitat/issues/6771
${hab_binary} pkg install core/hab-studio

echo "--- :habicat: Installed latest stable hab: $(${hab_binary} --version)"

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 13, 2019

Contributor

This should probably be moved into curlbash_hab for consistency.

local target=$1
local ident=$2
buildkite-agent meta-data set "hab-ident-${target}" "${ident}"
}

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 13, 2019

Contributor

I think these metadata functions aren't being used anymore. get_hab_ident and has_hab_ident aren't, at least, despite set_hab_ident being used.

It would be good to audit the rest and remove whatever's not being used anymore.

Copy link
Contributor

left a comment

Per offline discussion, let's merge the code as-is and do a follow-up PR to address the remaining concerns.

@scotthain scotthain merged commit a71b0ec into master Aug 13, 2019
5 checks passed
5 checks passed
DCO This commit has a DCO Signed-off-by
Details
buildkite/habitat-sh-habitat-master-verify Build #3047 passed (32 minutes, 15 seconds)
Details
buildkite/habitat-sh-habitat-master-website Build #176 passed (33 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 shain/iso_mac branch Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.