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

Add the ability to sync AlertManager configuration with Cortex #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add the ability to sync AlertManager configuration with Cortex #10

wants to merge 1 commit into from

Conversation

RyanW8
Copy link
Contributor

@RyanW8 RyanW8 commented Nov 18, 2020

No description provided.

@tomwilkie tomwilkie requested review from gotjosh, owen-d and jtlisi and removed request for gotjosh November 19, 2020 08:55
@jtlisi jtlisi self-assigned this Nov 19, 2020
Copy link
Contributor

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution @RyanW8! Looking good so far, a couple of small changes and a question.

@@ -13,6 +13,8 @@ This action is configured using environment variables defined in the workflow. T
| `CORTEX_API_KEY` | Optional password that is required for password protected Cortex clusters. An encrypted [github secret](https://help.github.com/en/actions/automating-your-workflow-with-github-actions/creating-and-using-encrypted-secrets ) is recommended. | `false` | N/A |
| `ACTION` | Which action to take. One of `lint`, `prepare`, `check`, `diff` or `sync` | `true` | N/A |
| `RULES_DIR` | Comma-separated list of directories to walk in order to source rules files | `false` | `./` |
| `COMPONENT` | `RULER` or `ALERTMANAGER`. This dictates whether to sync ruler rulegroups or AlertManager configurations | `true` | N/A |
| `ALERTMANAGER_CONFIG_PATH` | Path to the tenants AlertManager configuration | `true` If `COMPONENT` is set to `ALERTMANAGER` | `./alertManager.yml`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's missing a | at the end, in the diff, it looks like one column instead of two - am I missing something?

if [ -z "${RULES_DIR}" ]; then
echo "RULES_DIR not set, using './' as a default."
RULES_DIR="./"
fi

if [ ${COMPONENT} == "ALERTMANAGER" ] && [ -z "${ALERTMANAGER_CONFIG_PATH}" ]; then
echo "No ALERTMANAGER_CONFIG_PATH variable set. Defaulting to ./alertManager.yml"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
echo "No ALERTMANAGER_CONFIG_PATH variable set. Defaulting to ./alertManager.yml"
echo "ALERTMANAGER_CONFIG_PATH not set, using ./alertmanager.yml as a default."

if [ -z "${RULES_DIR}" ]; then
echo "RULES_DIR not set, using './' as a default."
RULES_DIR="./"
fi

if [ ${COMPONENT} == "ALERTMANAGER" ] && [ -z "${ALERTMANAGER_CONFIG_PATH}" ]; then
echo "No ALERTMANAGER_CONFIG_PATH variable set. Defaulting to ./alertManager.yml"
ALERTMANAGER_CONFIG_PATH=./alertManager.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ALERTMANAGER_CONFIG_PATH=./alertManager.yml
ALERTMANAGER_CONFIG_PATH=./alertmanager.yml

Comment on lines +31 to +32
echo "COMPONENT not set, select either RULER or ALERTMANAGER"
exit 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
echo "COMPONENT not set, select either RULER or ALERTMANAGER"
exit 1
echo "COMPONENT not set, using RULER as a default."
COMPONENT=RULER

Copy link
Contributor

Choose a reason for hiding this comment

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

This would make sure we do not break existing setups.

;;
esac
elif [ "${COMPONENT}" == "ALERTMANAGER" ]; then
OUTPUT=$(/usr/bin/cortextool alertmanager load ${ALERTMANAGER_CONFIG_PATH})
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have time to add the other commands just like the previous block?

;;
esac
elif [ "${COMPONENT}" == "ALERTMANAGER" ]; then
OUTPUT=$(/usr/bin/cortextool alertmanager load ${ALERTMANAGER_CONFIG_PATH})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
OUTPUT=$(/usr/bin/cortextool alertmanager load ${ALERTMANAGER_CONFIG_PATH})
verifyTenantAndAddress
OUTPUT=$(/usr/bin/cortextool alertmanager load ${ALERTMANAGER_CONFIG_PATH})

OUTPUT=$(/usr/bin/cortextool alertmanager load ${ALERTMANAGER_CONFIG_PATH})
STATUS=$?
else
echo "Invalid component selected, use RULER or ALERTMANAGER"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
echo "Invalid component selected, use RULER or ALERTMANAGER"
echo "COMPONENT is not valid, please use RULER or ALERTMANAGER."

STATUS=$?
;;
$PRINT_CMD)
OUTPUT=$(/usr/bin/cortextool rules print "$@")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
OUTPUT=$(/usr/bin/cortextool rules print "$@")
verifyTenantAndAddress
OUTPUT=$(/usr/bin/cortextool rules print "$@")

@jtlisi jtlisi removed their request for review February 5, 2021 13:57
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


RyanW8 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

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