search

CircleCI-Public / slack-orb

Create custom Slack notifications for CircleCI job statuses
https://circleci.com/developer/orbs/orb/circleci/slack
MIT License
214 stars 205 forks source link

Correct flow to check BASH first #84

Closed pbaderia01 closed 5 years ago

pbaderia01 commented 5 years ago

Orb version

2.1

What happened

The flow for status command does not look accurate. In the current flow we are first exporting env vars and then checking if bash exists. But if bash doesn't exist the env var export steps don't make much sense. https://github.com/CircleCI-Public/slack-orb/blob/c424f3f4bb4fa2d6ad8e0eeaaebff957a66fe95b/src/commands/status.yml#L79

Expected behavior

The flow, in my opinion, should be Check Bash Exists -> Check curl exists -> Export Env Vars -> Send notification script

pbaderia01 commented 5 years ago

Hi, I am picking this up. Please consider adding orbtoberfest label to the issue.

james-flynn-ie commented 5 years ago

I also added a PR for this, it includes a check to see if curl is installed after the check for bash: https://github.com/CircleCI-Public/slack-orb/pull/86

pbaderia01 commented 5 years ago

Hi @james-flynn-ie we already have a PR for curl. Please refer #81 and it would be great if you can suggest your changes on the existing PR. Thanks!

james-flynn-ie commented 5 years ago

Thanks for your comment, @piyush-insider .

I think for the purposes of Orbtoberfest they are trying to get as many different PRs as they can, just to see how many different approaches they can find.

From reading the rules, I don't think the PR needs to be closed (just a valid submission) so it shouldn't affect your entry. ;)

Good luck with the competition!

KyleTryon commented 5 years ago

This issue is currently being served by https://github.com/CircleCI-Public/slack-orb/pull/81