Skip to content

Conversation

@cameel
Copy link
Collaborator

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

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

4 participants