Skip to content
Snippets Groups Projects

Move gitlab.rb config library to the packages cookbook

Merged DJ Mountney requested to merge cleanup-config-settings into master
All threads resolved!

In this MR I had several goals.

Practical:

1. Establish a way in the config that we can use our parse_variables libraries for things like roles and secrets

My intent for roles it to create new roles library files under a libaries/roles folder, and call into those files using a predefined method like we do with parse_variables. That change will be in a seperate MR, but while working on it, I was working on roles in the package cookbook, but having our config still all handled by the gitlab cookbook seemed off, so I moved the gitlab.rb into packages.

I decided to create a new folder called config, and move our services in there, as well as the gitlab.rb. The intent is that when users go to add a new setting/service that they will remember to check to see if they need to update both files if they co-exist next to each other there in the config folder.

In order to use the other cookbooks libraries from the packages cookbook (which loads first), I define them in Procs, similar to the lazy syntax we are familiar with in chef.

2. Reduce the need to duplicate declaring the same config name multiple times when adding a new config object

While writing up the 'how to add new services' document, it became clear that it would be easier if we just used a method that we could define the gitlab attribute once. So I added an attribute method that takes care of setting the mixlib::config Mash for the attributes, and handles using the attribute when generating the Hash to pass back to the chef node.

Sugar:

3. Come up with a terse syntax for declaring the properties to minimise the perceived complexity when trying to add a new service

I have gone through a couple iterations already that achieved my other goals, but still looked very complicated as an observer after the fact. So I have added some additional "sugar" so it also looks less complex to use.

  • The Proc for the handler is set at the same time as the attribute, so in the docs we only have to document one place you need to add changes for your attribute, even if it needs parsing.
  • The attributes that need to be set on node['gitlab'] instead of just node are wrapped in a block that set that for you, while still using the same syntax

One of the previous iterations I had us declaring attributes similar to chef resources, where you passed a block, and within that block you could call methods for setting the parser etc. But this resulted in the list being a lot longer (because we couldn't one-line it) and it gave the impression of looking complicated to add an attribute. So I dropped it in favour of this one-line approach.

Fixes: https://gitlab.com/gitlab-org/omnibus-gitlab/issues/2662

Edited by DJ Mountney

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Marin Jankovski
  • Marin Jankovski
  • @twk3 I have few minor nitpicks, but the rest looks good. We should be able to merge this after the comments are addressed :)

  • assigned to @twk3

  • Marin Jankovski changed milestone to %10.0

    changed milestone to %10.0

  • DJ Mountney added 58 commits

    added 58 commits

    • 4ee1697e...c84f79dc - 57 commits from branch master
    • 72df11df - Merge remote-tracking branch 'origin/master' into cleanup-config-settings

    Compare with previous version

  • DJ Mountney added 3 commits

    added 3 commits

    • 847a6a90 - Implement review feedback
    • 3f8b5020 - Remove old git_http_server reference from the codebase
    • f5f8b959 - Cleanup refernces to generate hash

    Compare with previous version

  • DJ Mountney changed the description

    changed the description

  • DJ Mountney resolved all discussions

    resolved all discussions

  • DJ Mountney added 1 commit

    added 1 commit

    • 73300a7d - Add back in a empty GitLabEE module

    Compare with previous version

  • DJ Mountney added 1 commit

    added 1 commit

    • 6e991e5b - Use a new way of determining whether we are running the GitLab EE

    Compare with previous version

  • DJ Mountney added 1 commit

    added 1 commit

    • a5bf5df7 - Update error message spec for config

    Compare with previous version

  • assigned to @marin

  • Author Contributor

    @marin updated and ready for review again

  • DJ Mountney mentioned in merge request !1887 (merged)

    mentioned in merge request !1887 (merged)

  • Marin Jankovski
  • @twk3 one remaining comment, add a Changelog item and we can merge.

  • DJ Mountney resolved all discussions

    resolved all discussions

  • Author Contributor

    @marin updated

  • DJ Mountney added 58 commits

    added 58 commits

    • 4e02ad34...67210a1c - 57 commits from branch master
    • 50feabc9 - Merge branch 'master' into 'cleanup-config-settings'

    Compare with previous version

  • Marin Jankovski approved this merge request

    approved this merge request

  • Marin Jankovski mentioned in commit 3ba0f802

    mentioned in commit 3ba0f802

  • Please register or sign in to reply
    Loading