remind101 / slashdeploy

GitHub Deployments for Slack
https://slashdeploy.io
BSD 2-Clause "Simplified" License
153 stars 20 forks source link

Commit checks skipped when deploying to an environment with autodeploys set up #123

Closed russellballestrini closed 6 years ago

russellballestrini commented 6 years ago

Per @TerranceN:

When you slashdeploy from slack, if you're manually deploying to an environment that's autodeployed to, you're only notified that autodeploys are set up on that environment, and asked if you want to deploy anyway. When you say yes the deploy starts, regardless of the other commit checks (ex: tests passing).

On staging this is fine, but if the same behaviour happens on prod, and someone has to /deploy r101-api@master to production, it could lead to them accidentally skipping tests.

It’s unlikely, but it’d really suck to shoot ourselves in the foot this way someday.

Per @ejholmes:

Hmm, yeah, that seems like a bug. I thought the originaly behavior was to prompt if commit statuses weren't passing.

I think the correct behavior would be

  1. Prompt that the branch is configured to auto deploy
  2. Press "yes"
  3. Prompt that commit statuses aren't passing

Per @russellballestrini:

Likely related to this: https://github.com/remind101/slashdeploy/pull/100

We thought a race condition was the root cause: Sometimes (like once a day) if we send the list of context checks when requesting a Github Deployment, github will reject our attempt because of an apparent race condition related to "required context checks".

It's possible (or likely) that a repo configured for autodeployment triggers a deployment because all required_contexts move to success but then a subsequent commit pushed to the branch causes the github API to reset all check statuses, meanwhile the state held by slashdeploy regarding context checks and thus autodeployment are stale and show success when they should be reset to pending?

I think now that we have an Autodeployment watchdog, we should get revert the change in PR #100 because it was more of a work around than an actual fix.

russellballestrini commented 6 years ago

@ejholmes what sets the force param?:

https://github.com/remind101/slashdeploy/blob/3608dd37d5a1b02c27a64ddb6a71df2377804a86/app/commands/deploy_command.rb#L12-L18

Additionally it seems like we catch RedCommit (failed context checks) but when we create the MessageAction we pass force: true on the action?:

https://github.com/remind101/slashdeploy/blob/3608dd37d5a1b02c27a64ddb6a71df2377804a86/app/commands/deploy_command.rb#L57-L64