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.

Sign up for GitHub

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

Jump to bottom

store cli binaries as release assets #29

Merged
merged 5 commits into from Jan 30, 2023
Merged

store cli binaries as release assets #29

merged 5 commits into from Jan 30, 2023

Conversation

samsalisbury
Copy link
Contributor

@samsalisbury samsalisbury commented Jan 13, 2023 •
edited

Justification:

  • By storing release assets in GitHub Releases, we are now in a position to make a homebrew formula using official releases if we wanted, or to modify the action to make use of these prebuilt binaries to speed it up.

Also...

  • Running the release process was clunky when local tags didn't match those on the remote, that is now resolved.
  • In some cases the release workflow failed silently. We now check for failure explicitly in more places to prevent this in the future.

Summary:

  • Makes release assets built since build release binaries for all platforms #24 available from GitHub releases.
  • Ensures TMPDIR is set in more places where it's needed.
  • Ensures that local tags match remote tags before doing tag-related operations.

Quality:

@samsalisbury samsalisbury requested review from a team, alvin-huang and claire-labry and removed request for a team January 13, 2023 12:06
@samsalisbury samsalisbury marked this pull request as ready for review January 13, 2023 12:08
@samsalisbury samsalisbury changed the title upload release assets store cli binaries as release assets Jan 25, 2023
samsalisbury
samsalisbury commented Jan 25, 2023
View reviewed changes
Copy link
Contributor Author

@samsalisbury samsalisbury left a comment

Choose a reason for hiding this comment

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

Added a self-review to help other reviewers.

Makefile
@@ -135,18 +135,18 @@ env:

$(INITIAL_BUILD): $(SOURCE_ID)
@echo "# Running tests..." 1>&2
@$(RUN_TESTS_QUIET)
@BIN_PATH="$@" ./dev/build initial > /dev/null
@$(RUN_TESTS_QUIET) || exit 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These ... || exit 1 lines are to ensure that we fail early when appropriate.

Makefile
Comment on lines -276 to +279
@for Z in $^; do echo $$Z; done
@./dev/release/create $(RELEASE_ZIPS) || exit 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously release just listed the zips, now it creates a release including them as assets.

Makefile
Comment on lines -299 to +302
make changelog && \
$(MAKE) changelog && \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sub-makes should always be called using $(MAKE) as this preserves flags.

dev/build
Comment on lines +60 to +63
# Ensure TMPDIR is set.
: "${TMPDIR:="${RUNNER_TEMP:-}"}"
[[ -n "$TMPDIR" ]] || die "Neither TMPDIR nor RUNNER_TEMP is set."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This pattern is used all over the place. Locally TMPDIR is appropriate, unfortunately it's not set on GitHub runners, so we need to use their RUNNER_TEMP dir instead.

dev/release/create
@@ -16,6 +16,8 @@ for TAG in "${TAGS[@]}"; do
echo "$TAG"
done

git fetch --force --all --tags
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This ensures local tags match those on the remote.

@mdeggies mdeggies self-requested a review January 30, 2023 17:50
@mdeggies mdeggies merged commit 5d2cdd5 into main Jan 30, 2023
@mdeggies mdeggies deleted the save-release-assets branch January 30, 2023 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdeggies mdeggies mdeggies approved these changes

@alvin-huang alvin-huang Awaiting requested review from alvin-huang alvin-huang was automatically assigned from hashicorp/release-engineering

@claire-labry claire-labry Awaiting requested review from claire-labry claire-labry was automatically assigned from hashicorp/release-engineering

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@samsalisbury @mdeggies