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 new relic notification script post deploy. #27

Merged
merged 4 commits into from Jul 10, 2019

Conversation

3 participants
@jpetto
Copy link
Collaborator

commented Jul 3, 2019

Part of the fix for mozmeao/snippets-service#1055

@jpetto

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 3, 2019

@jgmize Would love a sanity check on this if/when you have time. Shell script isn't fully tested, but the intent should be pretty clear.

Would also like your thoughts on how/where to store/retrieve the New Relic API key.

Thanks!

NEWRELIC_USER=gitlab
# TODO: where/how to get newrelic api key?
# retrieve from somewhere on disk or a service?
API_KEY=idunno

This comment has been minimized.

Copy link
@jgmize

jgmize Jul 3, 2019

Member

API_KEY=$(< ~/.newrelic-api-key)

fi

# set the namespace before getting the deployment
kubectl config set-context $(kubectl config current-context) --namespace=$K8S_NAMESPACE

This comment has been minimized.

Copy link
@jgmize

jgmize Jul 3, 2019

Member

It would be simpler to omit this and use -n ${K8S_NAMESPACE} explicitly in the kubectl command below instead.

K8S_NAMESPACE=snippets-stage
elif [[ $NEWRELIC_APP = snippets-prod-frankfurt ]]; then
K8S_NAMESPACE=snippets-prod
fi

This comment has been minimized.

Copy link
@jgmize

jgmize Jul 3, 2019

Member

It may make more sense to set this explicitly in .gitlab-ci.yml

kubectl config set-context $(kubectl config current-context) --namespace=$K8S_NAMESPACE

# get the image value from the base deployment matching the K8S_NAMESPACE
FULL_IMAGE="kubectl get deploy ${K8S_NAMESPACE} -o jsonpath=\"{...image}\""

This comment has been minimized.

Copy link
@jgmize

jgmize Jul 3, 2019

Member

Nice use of jsonpath. :) While this works right now, it will break if we rename the deployment or use any sidecar containers in the future. I'm not sure if it's worth investing in future proofing it yet though.

This comment has been minimized.

Copy link
@jpetto

jpetto Jul 3, 2019

Author Collaborator

Would it be better to do the fully qualified path? Something like $.spec.template.spec.containers[0].image? But fine leaving as-is if it's too early for optimizing.

@jpetto

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 3, 2019

@jgmize Updated!

FULL_IMAGE="kubectl get deploy ${K8S_NAMESPACE} -n ${K8S_NAMESPACE} -o jsonpath=\"{$.spec.template.spec.containers[0].image}\""

# last 7 characters of the image value will be the hash
GIT_COMMIT_SHORT="${FULL_IMAGE: -7}"

This comment has been minimized.

Copy link
@glogiotatidis

glogiotatidis Jul 5, 2019

Member

You can also get GIT_COMMIT_SHORT from web-deploy.yaml using regex

grep -oP '(image: .+?:)\K.*' web-deploy.yaml

and in this case you'd call ./newrelic-notify.sh $(grep -oP '(image: .+?:)\K.*' web-deploy.yaml)

This avoids talking to k8s and adding another env variable in the gitlab config.

regex ftw!

jpetto added some commits Jul 3, 2019

Review fixes:
- Consolidate kubectl commands
- Get actual API key
- Update jsonpath to image to be more specific
Review fixes #2:
- Use regex to get image hash from yaml (instead of asking k8s)
- Alphabetize some things

@jpetto jpetto force-pushed the add-newrelic-post-deploy-notifications branch from 8f5a103 to 30f1007 Jul 8, 2019

@jpetto jpetto marked this pull request as ready for review Jul 8, 2019

@jpetto jpetto requested review from glogiotatidis and jgmize Jul 8, 2019

@@ -16,13 +16,15 @@ stages:
# tests.
- sleep 5s
- ./acceptance-tests.sh ${URL}
- ./newrelic-notify.sh

This comment has been minimized.

Copy link
@jgmize

jgmize Jul 8, 2019

Member

It might make more sense to move the newrelic notification before the sleep 5s

API_KEY=$(< ~/.newrelic-api-key)

# last 7 characters of the image value will be the hash
GIT_COMMIT_SHORT="grep -oP '(image: .+?:)\K.*' ${DEPLOYMENT}/web-deploy.yaml"

This comment has been minimized.

Copy link
@jgmize

jgmize Jul 8, 2019

Member

This is missing the $() wrapper needed for this command to execute


API_KEY=$(< ~/.newrelic-api-key)

# last 7 characters of the image value will be the hash

This comment has been minimized.

Copy link
@jgmize

jgmize Jul 8, 2019

Member

now that we're grepping the tag out of the deployment, this comment is less applicable

Review fixes part 3:
- Wrap subshell command to capture output
- Improve comments
- Notify new relic prior to waiting for/running acceptance tests
@jpetto

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 8, 2019

@jgmize updated!

@jpetto jpetto added this to Review in MozMEAO backend/infra Jul 8, 2019

@jgmize

jgmize approved these changes Jul 8, 2019

Copy link
Member

left a comment

👍

@jgmize jgmize merged commit dd35bba into master Jul 10, 2019

MozMEAO backend/infra automation moved this from Review to Complete Jul 10, 2019

@jgmize jgmize deleted the add-newrelic-post-deploy-notifications branch Jul 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.