Skip to content

Convert SubscriptionHistory create to an allowlist

Josianne Hyson requested to merge jh-subscription_history_allowlist into master

What does this MR do and why?

Addresses: #360628 (closed)

This MR:

  • Converts the GitlabSubscriptionHistory creation to use an allowlist of attributes rather than a denylist
  • Moves the creation logic for the history records to the history object

Prior to this change, if an attribute was added to the omit list and not tracked in the history table, it would cause issues with saving all GitlabSubscription records on the main stage. This is because the changes to the database are deployed across both stages, but the code changes are only deployed to the canary stage first. This means that the main stage is un-aware that it needs to omit the new attribute, as can be seen with this error here: https://sentry.gitlab.net/gitlab/gitlabcom/issues/3275019/?environment=gprd.

Converting this to an allowlist instead means that any new attributes will be ignored until the code hits the main deployment stage as well.

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

  1. Create a new field on the GitlabSubscription table
    • rails g migration AddTestFieldToGitlabSubscriptions test_field:boolean && rails db:migrate
  2. Run the spec, it should fail as the attribute is not in a known attribute constant
    • rspec ee/spec/models/gitlab_subscription_spec.rb
  3. Make changes to a subscription in the console - it should still be able to save
    subscription = GitlabSubscription.create!(namespace: Group.last)
    
    subscription.update!(test_field: false) # this should save successfully. If you repeat the process on master, this will error.
    GitlabSubscriptionHistory.last # should be the history logged for the previous change

MR acceptance checklist

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

Edited by Josianne Hyson

Merge request reports