-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
eks release workflow #405
eks release workflow #405
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments if you could address them
k8s/deployments/mpm-backend.yaml
Outdated
metadata: | ||
name: mpm-backend | ||
spec: | ||
progressDeadlineSeconds: 600 | ||
replicas: 1 | ||
revisionHistoryLimit: 10 | ||
selector: | ||
matchLabels: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, please do not change this file.
if kubectl get deployment mpm-backend; then | ||
kubectl rollout restart deployment/mpm-backend | ||
else | ||
echo "mpm-backend deployment does not exist, applying initial deployment..." | ||
kubectl apply -f k8s/deployments/mpm-backend.yaml | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need the if
/else
here. We always want to run both apply
and rollout restart
.
- The
apply
is needed such that changes to the kubernetesyaml
files are picked up. Which shouldn't happen that often. - The
rollout restart
is needed such that the new image gets deployed.
if kubectl get deployment mpm-frontend; then | ||
kubectl rollout restart deployment/mpm-frontend | ||
else | ||
echo "mpm-frontend deployment does not exist, applying initial deployment..." | ||
kubectl apply -f k8s/deployments/mpm-frontend.yaml | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add a step that runs apply
on services
.
name: Deploy to EKS | ||
|
||
on: | ||
release: | ||
types: | ||
- published | ||
workflow_dispatch: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having this as a separate workflow won't work. If a new release gets created this will run immediately. However, the deploy to DockerHub job takes around 1h usually. So, we Pull the Docker image before the new one is even available.
Could you instead, move the deploy
job into the already existing release-publish-dockerhub.yaml
, also rename it to deploy-demo
. Then please add a need
(see here) to depend on the release-publish
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Brief summary of the change made
Are there any other side effects of this change that we should be aware of?
Describe how you tested your changes?
Pull Request checklist
Please confirm you have completed any of the necessary steps below.