Fix accidental Mattermost enable on master + generic auto-vivification regression spec
What does this MR do?
Ports the master-side fix for the accidental Mattermost auto-enable bug from !9650 (merged) (which had only landed on 18-11-stable), and adds a generic regression spec so future deprecation-check helpers can't reintroduce the same class of bug.
Gitlab::Deprecations.remove_mattermost was reading config with plain []:
config = existing_config['mattermost']
return [] unless config.is_a?(Hash)
return [] unless config['enable'] == trueexisting_config is Gitlab['node'].normal, a Chef::Node::VividMash, where reading a missing key creates it. Since package::config does Gitlab[:node] = node, that object is the actual live attributes of the node being converged — the read silently vivified mattermost['enable'] => {}, which is truthy and could win against the real false default, turning Mattermost on for users who never configured it.
Commit 1 — cherry-pick from !9650 (merged)
Cherry-picks eb4afadb4605547bd65788f39f0b396e99ebb622 ("Do not enable Mattermost by accident") as-is, preserving authorship. Switches both reads in remove_mattermost to dig, matching every other helper in deprecations.rb.
Commit 2 — generic regression spec
Adds a new describe block in deprecations_spec.rb that:
- Snapshots
node.normalbefore/after runningGitlab::Deprecations.check_config(both:removaland:deprecation) on an emptyChef::Node, and asserts it is byte-for-byte unchanged. - Walks every
config_keyspath declared inGitlab::Deprecations.list, plus the hardcoded paths used byadditional_deprecationshelpers (mattermost, registry notifications, git_data_dirs, gitaly bundled-binaries/bin_path), asserting none of them get vivified.
This generalizes past the single mattermost case and will catch the same mistake in any future or existing deprecation-check helper, without needing to update the test when new deprecations are added.
I verified both new tests actually catch bugs (not just pass trivially) by running them against the unfixed remove_mattermost first — both failed with the exact vivification signature (mattermost['enable'] => {}) before the fix, and pass after.
Commit 3 - use key? check to avoid dig
Follow up item per Introduce the vivix fix for consitency on the m... (#10037 - closed) to remove the need for dib and subsequent Rubocop exclustion.
Related issues
Closes Introduce the vivix fix for consitency on the m... (#10037 - closed)
Verification
bundle exec rspec spec/chef/cookbooks/package/libraries/deprecations_spec.rb
# 32 examples, 0 failures
bundle exec rubocop files/gitlab-cookbooks/package/libraries/deprecations.rb spec/chef/cookbooks/package/libraries/deprecations_spec.rb
# 2 files inspected, no offenses detectedChecklist
See Definition of done.
For anything in this list which will not be completed, please provide a reason in the MR discussion.
Required
- MR title and description are up to date, accurate, and descriptive.
- MR targeting the appropriate branch.
- Latest Merge Result pipeline is green.
- When ready for review, MR is labeled workflowready for review per the Distribution MR workflow.
- The UBT version and corresponding checksum hash have been updated and referenced in the merge request if applicable. - n/a
- UBT EE pipeline (
Trigger:ee-package-ubt) is green
- UBT EE pipeline (
For GitLab team members
If you don't have access to this, the reviewer should trigger these jobs for you during the review process.
- The manual
Trigger:ee-packagejobs have a green pipeline running against latest commit.- To debug QA failures, refer to the Investigate QA failures section.
- If
config/softwareorconfig/patchesdirectories are changed, make sure thebuild-package-on-all-osjob within theTrigger:ee-packagedownstream pipeline succeeded. - If you are changing anything SSL related, then the
Trigger:package:fipsmanual job within theTrigger:ee-packagedownstream pipeline must succeed. -n/a - If CI configuration is changed, the branch must be pushed to
dev.gitlab.orgto confirm regular branch builds aren't broken. - n/a
Expected (please provide an explanation if not completing)
- Test plan indicating conditions for success has been posted and passes.
- Documentation created/updated. -n/a
- Tests added.
- Integration tests added to GitLab QA. -n/a
- Equivalent MR/issue for the GitLab Chart opened. - n/a