Skip to content

GitLab Next

  • Projects
  • Groups
  • Snippets
  • Help
    • Loading...
  • Help
    • Help
    • Support
    • Community forum
    • Submit feedback
    • Contribute to GitLab
  • Sign in / Register
GitLab
GitLab
  • Project overview
    • Project overview
    • Details
    • Activity
    • Releases
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributors
    • Graph
    • Compare
    • Locked Files
  • Issues 35,875
    • Issues 35,875
    • List
    • Boards
    • Labels
    • Service Desk
    • Milestones
    • Iterations
  • Merge Requests 1,262
    • Merge Requests 1,262
  • Requirements
    • Requirements
    • List
  • CI / CD
    • CI / CD
    • Pipelines
    • Jobs
    • Schedules
    • Test Cases
  • Operations
    • Operations
    • Metrics
    • Incidents
    • Environments
  • Packages & Registries
    • Packages & Registries
    • Container Registry
  • Analytics
    • Analytics
    • CI / CD
    • Code Review
    • Insights
    • Issue
    • Repository
    • Value Stream
  • Snippets
    • Snippets
  • Members
    • Members
  • Activity
  • Graph
  • Create a new issue
  • Jobs
  • Commits
  • Issue Boards
Collapse sidebar
  • GitLab.org
  • GitLabGitLab
  • Merge Requests
  • !31005

Merged
Opened May 04, 2020 by Nick Thomas@nick.thomas🏖Maintainer0 of 13 tasks completed0/13 tasks

Set MergeRequestDiff#external_diff_store in more cases

  • Overview 22
  • Commits 1
  • Pipelines 7
  • Changes 4

What does this MR do?

Merge request diffs can store their data either in-database, or in an external file. That file may be on disk (recommended against) or in object storage (enabled on GitLab.com today).

We've had a couple of reports where the merge_request_diff.external_diff_store column has had the wrong value for some diffs. The practical outcome of this is that the MR "changes" tab cannot be displayed - the file is correctly stored in object storage, but since we don't record 2 in this column, we don't know to look it up from there.

This MR spots and remedies a couple of deficiencies in when we set this value.

Excluding it during project import is unnecessary, because importing a project actually persists all the MR diffs as in-database ones, that are then migrated to become external storage diffs. This is something we need to improve, so they go straight to object storage, but in the meantime, we can clean this up.

Recalculating every time stored_externally is changed, in addition to external_diff (this is just the filename) changing, seems to be the cleanest way to plug the gap.

On GitLab.com, and presumably on a range of other instances too, we now have records that need fixing - this can be done as a follow-up MR once this one is merged. We shouldn't close the associated issue until this is done.

Since we're trying to roll out external diffs on GitLab.com, per gitlab-com/gl-infra/production#1851 (closed) , we may want to Pick into auto-deploy to ensure this fix reaches GitLab.com sooner.

Screenshots

Does this MR meet the acceptance criteria?

Conformity

  • Changelog entry
  • Documentation (if required)
  • Code review guidelines
  • Merge request performance guidelines
  • Style guides
  • Database guides
  • Separation of EE specific content

Availability and Testing

  • Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process.
  • Tested in all supported browsers
  • Informed Infrastructure department of a default or new setting change, if applicable per definition of done

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

Part of #216216 (closed)

Closes #197317 (closed)

Edited May 05, 2020 by Nick Thomas
Assignee
Assign to
Reviewer
Request review from
13.0
Milestone
13.0 (Past due)
Assign milestone
Time tracking
Reference: gitlab-org/gitlab!31005
Source branch: 216216-fix-mr-diffs-external-diffs-store-bug