Skip to content

Conversation

@zimeg
Copy link
Member

@zimeg zimeg commented Apr 2, 2025

Summary

This PR uses a packaged CLI build in CI runs to match releases, using the -s and -W flags to omit debug info 🤖

Preview

The updated build is shown in the download size!

Before changes:

$ du dist/*
25276   dist/slack-macos_darwin_amd64_v1/bin
25248   dist/slack_linux_amd64_v1/bin
25592   dist/slack_windows_amd64_v1/bin

After changes:

$ du dist/*
17768   dist/slack-macos_darwin_amd64_v1/bin
17352   dist/slack_linux_amd64_v1/bin
17712   dist/slack_windows_amd64_v1/bin

Notes

A few comments were exploring other CI happenings that other PRs hope to address 🙏

Requirements

@zimeg zimeg added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented code health M-T: Test improvements and anything that improves code health semver:patch Use on pull requests to describe the release version increment labels Apr 2, 2025
@zimeg zimeg added this to the Next Release milestone Apr 2, 2025
@zimeg zimeg self-assigned this Apr 2, 2025
@codecov
Copy link

codecov bot commented Apr 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.78%. Comparing base (f2f33c4) to head (67bc246).
Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main       #6   +/-   ##
=======================================
  Coverage   62.78%   62.78%           
=======================================
  Files         210      210           
  Lines       22053    22053           
=======================================
  Hits        13846    13846           
- Misses       7128     7129    +1     
+ Partials     1079     1078    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member Author

@zimeg zimeg left a comment

Choose a reason for hiding this comment

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

📝 Leaving notes with findings in CI as findings happen!

Copy link
Member Author

@zimeg zimeg left a comment

Choose a reason for hiding this comment

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

🏁 The describe command and --exclude patterns have become a puzzle to me...

I made a note of some strangeness in finding feature builds and am updating the CI to support the current tagging behavior we have elsewhere.

This might be interesting to revisit if feature builds start to make versions confusing IMO, but otherwise I want to check one more thing in this PR? 👾

Copy link
Member Author

@zimeg zimeg left a comment

Choose a reason for hiding this comment

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

🎁 A perhaps excited change to the packing was found!

I'm going to retry the testing CI with these changes but without saved artifacts to mimic a dev-build best. Otherwise I'm hoping this is soon ready for review 🙏 ✨

command: |
export LDFLAGS="-s -w -X 'github.com/slackapi/slack-cli/internal/pkg/version.Version=$BUILD_VERSION'"
make build-snapshot
make LDFLAGS="$LDFLAGS" build-snapshot
Copy link
Member Author

Choose a reason for hiding this comment

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

The -s and -w flag weren't being used when building the snapshot! I'm hoping this improves our runtime speeds around 👾

Before:

$ du dist/*
...
13324   dist/slack_cli_3.0.2-main-build-test-feature-6-gcc496f3_linux_64-bit.tar.gz
13332   dist/slack_cli_3.0.2-main-build-test-feature-6-gcc496f3_macOS_64-bit.tar.gz
14376   dist/slack_cli_3.0.2-main-build-test-feature-6-gcc496f3_windows_64-bit.zip

After:

$ du dist/*
...
7156    dist/slack_cli_3.0.2-main-build-test-feature-6-gcc496f3_linux_64-bit.tar.gz
7252    dist/slack_cli_3.0.2-main-build-test-feature-6-gcc496f3_macOS_64-bit.tar.gz
7208    dist/slack_cli_3.0.2-main-build-test-feature-6-gcc496f3_windows_64-bit.zip

Copy link
Member

Choose a reason for hiding this comment

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

Great catch @zimeg! 👏🏻 Before this change, the export LDFLAGS was unused and instead it was using the LDFLAGS set by Makefile's startup.

@zimeg
Copy link
Member Author

zimeg commented Apr 2, 2025

🗣️ I'm wanting to avoid more updates in this PR and am thinking this might have a fix for the dev-build breaking with previous feature builds, but am open to follow up and continued changes as needed! Marking this "ready for review"-

@zimeg zimeg marked this pull request as ready for review April 2, 2025 05:31
@zimeg zimeg requested a review from a team as a code owner April 2, 2025 05:31
@zimeg zimeg changed the title ci: include the latest archives in development build releases ci: include compiled archives in development build releases Apr 2, 2025
@zimeg zimeg marked this pull request as draft April 2, 2025 16:59
@zimeg zimeg changed the title ci: include compiled archives in development build releases ci: include compiled archives in snapshot build releases Apr 2, 2025
@zimeg zimeg marked this pull request as ready for review April 2, 2025 23:24
Copy link
Member

@mwbrooks mwbrooks left a comment

Choose a reason for hiding this comment

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

✅ LGTM! Thanks for catching this oversight. 👏🏻

❓ In a previous commit, you also updated scripts/archive.sh with set -eo. Just curious why you decided to remove it from this PR?

@zimeg
Copy link
Member Author

zimeg commented Apr 3, 2025

@mwbrooks And likewise, thank you for the review and catching that comment 👾 ✨

...updated scripts/archive.sh with set -eo...

That change was muddled with excluding feature tags from versions! With it, the archive.sh script would error and exit if a feature tag existed on main . Some feature tags were used in testing and this confirmed CI failed for these cases when exploring things 🫡

I'm meaning to move this into a separate PR since I believe it's a change that might need to be revisited or explored more!

For now though, I notice our dev-build has the expected assets without the mentioned tag strangeness! I'll merge this and will check back on these assets and follow up with the error PR.

@zimeg zimeg merged commit a1ee7f9 into main Apr 3, 2025
6 checks passed
@zimeg zimeg deleted the zimeg-build-dev branch April 3, 2025 17:09
@zimeg zimeg added build M-T: Changes to compilation and CI processes and removed bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented code health M-T: Test improvements and anything that improves code health labels Apr 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build M-T: Changes to compilation and CI processes semver:patch Use on pull requests to describe the release version increment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants