Skip to content

Conversation

@haidargit
Copy link
Contributor

Summary

Hi,
This PR improves the console output in the Deno installation part by adjusting the version check logic to handle string values (e.g., v2.3.1). Replaced numeric comparison with a neutral string check. Thanks.

Screenshot from 2025-05-14 10-27-25

Requirements

@haidargit haidargit requested a review from a team as a code owner May 14, 2025 04:29
@salesforce-cla salesforce-cla bot added the cla:missing The CLA was not signed label May 14, 2025
@salesforce-cla
Copy link

Thanks for the contribution! Before we can merge this, we need @haidargit to sign the Salesforce Inc. Contributor License Agreement.

@zimeg
Copy link
Member

zimeg commented May 14, 2025

Hey @haidargit! 👋 Thanks so much for catching this and sending in a fix 🤖 ✨

Before we start review, could you sign the CLA linked from @salesforce-cla? We might have to poke the bot once this is done 😉

@haidargit
Copy link
Contributor Author

Hi @zimeg 👋, Thanks for the reviews.

Sure. I just signed the CLA agreement, but it only shows the message:
You already signed the CLA on 2025-05-14

@zimeg zimeg closed this May 14, 2025
@zimeg zimeg reopened this May 14, 2025
@salesforce-cla salesforce-cla bot added cla:signed and removed cla:missing The CLA was not signed labels May 14, 2025
@zimeg zimeg added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented changelog Use on updates to be included in the release notes semver:patch Use on pull requests to describe the release version increment labels May 14, 2025
@zimeg zimeg added this to the Next Release milestone May 14, 2025
@zimeg zimeg changed the title chore: deno installation console output improvement fix: parse the latest deno release in installation script outputs as a semantic version May 14, 2025
Copy link
Member

@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.

@haidargit Super appreciative of this change! Thanks again for sharing a fix to both the production and development scripts 🙏

I just confirmed this prints both the correct and missing versions as expected:

🔍 Searching for the latest released Deno version... Found: v2.3.1
🔍 Searching for the latest released Deno version... Found: Unknown

For reference of other reviewers, changing the api.github.com URL is sometimes enough to get an unexpected response 👾

More changes to this script might soon follow, but for now this makes for a much better experience. Let's go ahead and merge this for the next release! 🚢 💨

@zimeg zimeg changed the title fix: parse the latest deno release in installation script outputs as a semantic version fix: parse the latest deno release in installation script outputs as a tagged version May 14, 2025
@zimeg
Copy link
Member

zimeg commented May 14, 2025

📝 But before we merge, I forget that semantic versions don't include the v prefix! Oops!

@zimeg zimeg merged commit 1129877 into slackapi:main May 14, 2025
5 of 6 checks passed
@codecov
Copy link

codecov bot commented May 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.17%. Comparing base (6ffb164) to head (1678049).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #91   +/-   ##
=======================================
  Coverage   63.17%   63.17%           
=======================================
  Files         210      210           
  Lines       22186    22186           
=======================================
  Hits        14017    14017           
  Misses       7082     7082           
  Partials     1087     1087           

☔ 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.

@zimeg
Copy link
Member

zimeg commented May 14, 2025

📝 I took a note that the delete pre-release when a branch is deleted job was stuck in a failed state after reopening this PR for CLA reasons.

I'm wondering if we can make this check pass somehow, but for now that ought not be a blocker!

@haidargit
Copy link
Contributor Author

appreciate, @zimeg and other team! Thanks ✨

@mwbrooks
Copy link
Member

Thanks a lot for the fix @haidargit 🙇🏻 Really appreciate your attention to detail in both the code (both scripts), PR title (convention commits), and PR description (screenshot) 👌🏻 👏🏻

@mwbrooks
Copy link
Member

@haidargit Today we prepared Release v3.2.0.

Next week our QA team will review it and then we'll set it as the latest release for the install scripts & slack upgrade command. This should happen next Thursday, May 22nd.

Your install script changes are already live for both the dev build and production build 🎉 You can try them out below:

Latest Release Candidate (Dev Build):

$ curl -fsSL https://downloads.slack-edge.com/slack-cli/install-dev.sh | bash -s slack-dev

$ slack-dev --version
Using slack-dev v3.2.0

Latest Stable Release:

$ curl -fsSL https://downloads.slack-edge.com/slack-cli/install.sh | bash

$ slack --version
Using slack v3.1.0

Thanks again for the contribution 🙇🏻 At the bottom of Release v3.2.0 you're also listed as a first-time contributor 😄

@haidargit
Copy link
Contributor Author

Thanks for merging this @mwbrooks, @zimeg, and slack-api team ⭐
Really cool to be part of contributor. Appreciate it! 🙇🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented changelog Use on updates to be included in the release notes cla:signed 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