Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ha roles #186

Merged
merged 11 commits into from
Apr 3, 2018
Merged

Add ha roles #186

merged 11 commits into from
Apr 3, 2018

Conversation

LongLiveCHIEF
Copy link
Contributor

This pull request adds the roles configuration item for controlling scaled instances of GitLab Omnibus.

Copy link
Member

@eputnam eputnam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some overlooked whitespace and a few rubocop offenses to review

.travis.yml Outdated
@@ -2,6 +2,7 @@
sudo: false
dist: trusty
language: ruby

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix.

@@ -75,6 +75,7 @@
$backup_cron_enable = $::gitlab::backup_cron_enable
$backup_cron_minute = $::gitlab::backup_cron_minute
$backup_cron_hour = $::gitlab::backup_cron_hour
$roles = $::gitlab::roles
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a way to make this var name more descriptive? perhaps $ha_roles or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the most part, this module uses variables that are consistent with the names of the config keys expected by the product. This allows users to reference the product documentation for how a config is expected to change the behavior of the product, and removes ambiguity/uncertainty by the implementor on whether or not our name for a config is really some other config for the gitlab.rb.template, which is what this module configures.

@@ -247,6 +247,11 @@
# Default: false
# Replicate the registry Nginx config from the Gitlab Nginx config.
#
# [*roles*]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs! \o/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my comment above. The standard for this module is to document something that is not a feature of the omnibus package configuration. Documenting every config would be redundant, and could cause unexpected problems when gitlab changes the behavior or possible sub-key values.

There fore, the documentation provided for this param is adequate and in keeping with the standards of this module.

@eputnam eputnam added the enhancement New feature or request label Feb 26, 2018
@LongLiveCHIEF
Copy link
Contributor Author

@eputnam are you familiar with the product itself? It seems like some of the review items you raised are indicating that you're unaware that this module is basically puppetizing a chef configuration, and that we're merely automating the generation of that config file according to the docs maintained by the vendor.

We can't be the source of documentation for these configs, because it creates too many issues when debugging configuration problems. When we automate some manual part of the omnibus package/service, then we document it... otherwise we merely indicate an expected type for each param, and in the param docs we point to the usage of that config by the vendor.

It's confusing unless you're familiar with how GitLab Omnibus is administered.

@LongLiveCHIEF LongLiveCHIEF dismissed eputnam’s stale review February 26, 2018 20:29

Discussion concerns are within standards and are consistent with user expectations for implementing this module.

@LongLiveCHIEF
Copy link
Contributor Author

@eputnam a good example of how we document non gitlab.rb.template features is actually present in one of my other open Merge Requests: Db indexing for git authorized keys.

This is a feature of GitLab that requires manual configuration by the installer, (whereas the majority are handled by the bundled chef process according to the settings realized in gitlab.rb). As such, I added a section to the README.md about enabling it in the module, in addition to the param docs.

Make sense? Sound good? Maybe we should actually document this distinction in the README so that module consumers don't have to figure out implicitly how our documentation works?

@LongLiveCHIEF LongLiveCHIEF added this to the Gitlab HA Support milestone Mar 30, 2018
@LongLiveCHIEF LongLiveCHIEF merged commit 3b2fc6c into voxpupuli:master Apr 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants