Skip to content

Add more guidance about safety of Gitaly bumps

What does this MR do and why?

Helps authors and reviewers get Gitaly version bumps merged, faster and with less risk.

Original discussion

!133829 (comment 1598660740):

Updating the Gitaly Ruby Gem is safe as long as two things are given:

  • No definitions that have been removed in the version bump are in use on the Rails side.
  • No definitions that have been added in the version bump become used without a feature flag.

The first one should be catched via specs, and we don't add any new usage sites here. So yes, this bump looks good to me, thanks!

This domain expert comment is super helpful, and I think it removes the need for a back and forth before approval.

thought (non-blocking): Should we have some kind of guideline/danger section/automation to prompt this kind of statement of reasoning on all similar MRs, upfront?

Author Maintainer

Thanks @mkozono!

This merge request requires coordination with gitaly deployments. Before merging this merge request we should verify that gitaly running in production already implements the new gRPC interface included here.

Failing to do so will introduce a [non backward compatible change]? (https://docs.gitlab.com/ee/development/multi_version_compatibility.html) during canary depoyment that can cause an incident.

  1. Identify the gitaly MR introducing the new interface
  2. Verify that the environment widget contains a gprd deployment

I suppose maybe this comment from Dangerbot could be updated to include more detail. Any thoughts @pks-gitlab?

Developer

@mkozono @dskim_gitlab That would be a great addition indeed. I ain't really got much of a clue where these messages are maintained, but I'd be happy to review any MR that does address this.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Merge request reports