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

[CI] Reusable command for gitter notifications #12182

Merged
merged 2 commits into from
Oct 25, 2021

Conversation

cameel
Copy link
Member

@cameel cameel commented Oct 22, 2021

This is a simple refactor, extracted into a separate PR because I need it in more than one other PR.

It converts the existing notification step templates into a parameterized and reusable command. This reduces duplication and makes it easier to add new notifications (which is needed in #12181).

@cameel cameel self-assigned this Oct 22, 2021
@cameel cameel force-pushed the circleci-gitter-notification-command branch from c8ada5e to 044f418 Compare October 22, 2021 16:38
@cameel cameel force-pushed the circleci-gitter-notification-command branch from db5d6e5 to a4fce30 Compare October 22, 2021 19:28
@hrkrshnn
Copy link
Member

@cameel Was this tested by any chance? Looks good. Can approve if this was tested. We can also test in prod :)

@Marenz
Copy link
Contributor

Marenz commented Oct 25, 2021

Yeah, I think the easiest is to test it in production, as it explicitly excludes PRs. Though of course you could add a test-commit that removes that exception so it runs on PRs as well.

Copy link
Contributor

@Marenz Marenz left a comment

Choose a reason for hiding this comment

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

LGTM. I am okay with production testing, but a simple commit that removes the PR exclusion might also do the trick to test it

@cameel
Copy link
Member Author

cameel commented Oct 25, 2021

@hrkrshnn I did test it but not very extensively (I didn't want to spam the channel and also the variables are going to be different in non-PR runs anyway so it's not foolproof). I tested it together with #12181. Take a look at #12181 (comment).

@cameel
Copy link
Member Author

cameel commented Oct 25, 2021

@Marenz I did that in #12181 (it was originally a part of that PR) and when it worked I removed the debug commits.

@hrkrshnn hrkrshnn merged commit 8460a65 into develop Oct 25, 2021
@hrkrshnn hrkrshnn deleted the circleci-gitter-notification-command branch October 25, 2021 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants