Skip to content

[DISCO-2169] Delete CircleCI approval task for prod deployment #250

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

Merged
merged 11 commits into from
Mar 30, 2023
Merged

Conversation

Trinaa
Copy link
Contributor

@Trinaa Trinaa commented Mar 24, 2023

References

JIRA: DISCO-2169
GitHub: N/A

Description

Remove the 'unhold-to-deploy-to-prod' CircleCI step in order to fulfill Rapid Release.

PR Review Checklist

Put an x in the boxes that apply

  • This PR conforms to the Contribution Guidelines
  • The PR title starts with the JIRA issue reference, format [DISCO-####], and has the same title (if applicable)
  • [do not deploy] and [load test: (abort|warn)] keywords are applied (if applicable)
  • Documentation has been updated (if applicable)
  • Functional and performance test coverage has been expanded and maintained (if applicable)

@Trinaa Trinaa requested review from ncloudioj and hackebrot March 24, 2023 16:16
@Trinaa
Copy link
Contributor Author

Trinaa commented Mar 24, 2023

@ncloudioj & @hackebrot
Does this change fulfill the task? Do we need to sync with SRE on anything?

@hackebrot
Copy link
Contributor

Hi @Trinaa! 👋🏻

It would be good to sync with Services SRE on this change, yes.

We definitely also want to update any documentation that references the CircleCI approval task and ensure that everybody on the team merging pull requests to the main branch is aware that this triggers a prod deployment and requires active monitoring.

@ncloudioj
Copy link
Contributor

Let's also update the deployment doc to reflect this change.

Copy link
Contributor

@hackebrot hackebrot left a comment

Choose a reason for hiding this comment

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

LGTM! 👍🏻

@hackebrot
Copy link
Contributor

hackebrot commented Mar 27, 2023

Please don't merge this pull request just yet, @Trinaa. ⚠️

The changes look good to me, but I'd like to make sure everyone is aware of this change to the release process.

@mtbottle
Copy link
Contributor

As I understand, docker-image-publish-prod does a deploy to both staging and prod and docker-image-publish-staging also does a deploy to staging. I don't think it makes sense to have 2 staging deploys, so I would vote to remove one of them.

@ncloudioj
Copy link
Contributor

As I understand, docker-image-publish-prod does a deploy to both staging and prod and docker-image-publish-staging also does a deploy to staging. I don't think it makes sense to have 2 staging deploys, so I would vote to remove one of them.

Good call out. Yup, I think we can drop the second stage deployment from the pipeline. This needs SRE's help for reconfiguration.

@Trinaa
Copy link
Contributor Author

Trinaa commented Mar 27, 2023

As I understand, docker-image-publish-prod does a deploy to both staging and prod and docker-image-publish-staging also does a deploy to staging. I don't think it makes sense to have 2 staging deploys, so I would vote to remove one of them.

Good call out. Yup, I think we can drop the second stage deployment from the pipeline. This needs SRE's help for reconfiguration.

Spoke to @dlactin about this earlier, he is already on it 🚀

@Trinaa
Copy link
Contributor Author

Trinaa commented Mar 28, 2023

@hackebrot @ncloudioj
Following SRE's recent change, disabling Docker image validation in Merino-py, I am able to set the creation and publication of the load test Docker image as a dependency to publishing the stage and production Merino Docker images.

I have also consolidated the docker-image-publish-stage and docker-image-publish-prod steps.

Please have a look

Proposed new CircleCI flow:
image

Copy link
Contributor

@ncloudioj ncloudioj left a comment

Choose a reason for hiding this comment

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

🚢

Copy link
Contributor

@hackebrot hackebrot left a comment

Choose a reason for hiding this comment

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

Great work, @Trinaa! 🚀

@quiiver
Copy link
Collaborator

quiiver commented Mar 29, 2023

AFAICT, there is no stage health check required before deploying to production. Is that the case? If so, it concerns me that we could deploy to prod with staging broken.

@ncloudioj
Copy link
Contributor

ncloudioj commented Mar 29, 2023

AFAICT, there is no stage health check required before deploying to production. Is that the case? If so, it concerns me that we could deploy to prod with staging broken.

Good point, @quiiver. We currently only have a health check against prod-canary, if the check fails, the production deployment will abort. @Trinaa filed this followup ticket (DISCO-2322) to add some basic smoke tests for staging. I think we can merge this as is and keep a closer eye on how it goes in the wild.

Does that make sense?

@Trinaa
Copy link
Contributor Author

Trinaa commented Mar 29, 2023

Follow-up: DISCO-2322 Add smoke tests to Merino CD pipeline.

Also looking at the merino Jenkinsfile-py I think the canary also applies to stage.

@Trinaa Trinaa added this pull request to the merge queue Mar 30, 2023
Merged via the queue into main with commit 7096057 Mar 30, 2023
@Trinaa Trinaa deleted the disco-2169 branch April 4, 2023 22:22
@Trinaa Trinaa restored the disco-2169 branch April 4, 2023 22:23
@Trinaa Trinaa deleted the disco-2169 branch April 4, 2023 22:23
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.

5 participants