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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a GitHub deployment for the batch head #982

Merged
merged 2 commits into from Jan 10, 2020

Conversation

@DazWorrall
Copy link
Member

DazWorrall commented Jan 9, 2020

It's not currently possible for other apps to identify deployments associated with the head of the deploy batch - we create one for the PR head of any included pull requests, so context appears in the PR timeline, but this is not sufficient to to discover the full range of commits. This PR creates a deployment for the head of the batch being deployed, in addition to the deployments for each PR.

Some refactoring was required here:

  • Remove the CommitDeployment#commit association (I will follow up with a schema cleanup once this has shipped successfully, though I may need a release cutting first)
  • Add sha to CommitDeployment
  • Handle deployment creation in a job rather than inline - this code path now queries GitHub for PR head refs, and that should not be done in the critical path of creating a Deploy

I have 馃帺ed this locally.

@DazWorrall DazWorrall requested review from JackTLi and casperisfine Jan 9, 2020
def change
add_column :commit_deployments, :sha, :string, limit: 40
add_index :commit_deployments, :sha
add_index :commit_deployments, [:task_id, :sha], unique: true

This comment has been minimized.

Copy link
@casperisfine

casperisfine Jan 9, 2020

Contributor

Couple questions here:

  • Is this index (these indexes) actually needed? When do we query commit_deployments ?
  • Should we backfill the existing records, and then make sha NOT NULL ?

This comment has been minimized.

Copy link
@DazWorrall

DazWorrall Jan 9, 2020

Author Member

Is this index (these indexes) actually needed? When do we query commit_deployments ?

Good question. I鈥檝e added these for parity with the existing association (which has equivalent indexes on commit_id), but will do some investigation into their use.

Should we backfill the existing records, and then make sha NOT NULL ?

I plan this for the follow up - if I do that now, we will run the migration first and backfill the existing data, but any deployments created between db:migrate and the new code deploy would be created with a null sha. For safety we need to backfill the data after the code has been deployed.

This comment has been minimized.

Copy link
@DazWorrall

DazWorrall Jan 10, 2020

Author Member

Ok I did some digging - the commit_id index is used only in one place that I can see, in DestroyStackJob. I changed that to search by task_id instead, and have removed that new index.

I'm not sure what the unique index was guarding - it doesn't look like we were catching RecordNotUnique anywhere in the call path, and I don't see any exceptions in Bugsnag. I've removed the new unique index, we can add it back if it causes problems in production.

This comment has been minimized.

Copy link
@casperisfine

casperisfine Jan 10, 2020

Contributor

馃憤

Copy link
Contributor

casperisfine left a comment

Minor concerns about the migration, but other than that looks really good. I especially like the new job.

@JackTLi
JackTLi approved these changes Jan 9, 2020
@DazWorrall DazWorrall force-pushed the deployment-for-batch-head branch from 99a698d to cc0c123 Jan 10, 2020
@DazWorrall DazWorrall merged commit 5eabcfb into master Jan 10, 2020
3 checks passed
3 checks passed
CLA Contributor License Agreement (CLA) status
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@DazWorrall DazWorrall deleted the deployment-for-batch-head branch Jan 10, 2020
@shopify-shipit shopify-shipit bot deployed to rubygems Jan 10, 2020 Active
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.