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

Allow configuration of the alertmanager via puppet. #267

Merged
merged 2 commits into from
Aug 30, 2018

Conversation

gfokkema
Copy link
Contributor

@gfokkema gfokkema commented Aug 29, 2018

Pull Request (PR) description

This PR adds support for configuring the Alertmanager gitlab service.
Documentation for that service is still quite sparse, best I've come across is:
https://prometheus.io/docs/alerting/alertmanager/

Right now our gitlab server is perpetually in error due to an unconfigured alertmanager,
and there's no way to disable it (this commit adds that).

Disabling alertmanager via hiera:

gitlab::alertmanager:
  enable: false

This Pull Request (PR) fixes the following issues

Fixes #266

Couldn't find any unit tests for other params like this, so I haven't included them here either.
Please let me know :)

@bastelfreak bastelfreak added enhancement New feature or request needs-tests needs-work not ready to merge just yet labels Aug 29, 2018
@bastelfreak
Copy link
Member

Hi @gfokkema, thanks for the PR. Can you take a look at the used email address in the commit? It is not associated with your github accoun. Can you please also add a unit test for this?

@gfokkema
Copy link
Contributor Author

gfokkema commented Aug 29, 2018

Dear @bastelfreak, in my opinion it's better to make changes according to the style of the file.
Should you want empty hashes instead of undefs, I'd propose doing that in a separate PR for all 40-ish Optional[Hash] values at once so the file stays consistent.

The same goes for unit tests regarding the Optional[Hash] values.

I've corrected the email address, that was by mistake.

@gfokkema
Copy link
Contributor Author

gfokkema commented Aug 29, 2018

Dear @bastelfreak I think it'd be more appropriate to label this as a bug as well.
This issue prevents a fresh gitlab install by puppet from starting correctly via systemd (gitlab-runsvdir).

@LongLiveCHIEF LongLiveCHIEF removed the needs-work not ready to merge just yet label Aug 30, 2018
@LongLiveCHIEF
Copy link
Contributor

@gfokkema it's not really a bug. I classify a bug label to be equivalent with a piece of code in this module that doesn't work as intended.

As Gitlab continues to drop new releases each month, there are times when they add new configurations that outpace the configurations available in this module.

There's nothing wrong with the code in this moudle (at least in regards to this PR), it just doesn't yet support the latest versions of GitLab Omnibus. Therefore it's an enhancement, and not a bug.

When we add these, we need to make sure they don't cause errors with installations of older versions of GitLab as well, or that could cause a bug.

@LongLiveCHIEF
Copy link
Contributor

LongLiveCHIEF commented Aug 30, 2018

This issue prevents a fresh gitlab install by puppet from starting correctly via systemd (gitlab-runsvdir).

@gfokkema can you open an issue with the error reported by systemd? The error reported in #266 is a result of trying to utilize a parameter this module does not yet support, and has nothing to do with systemd.

@LongLiveCHIEF LongLiveCHIEF merged commit 756cb83 into voxpupuli:master Aug 30, 2018
@LongLiveCHIEF
Copy link
Contributor

@gfokkema thanks for the PR! This will be included in the next release ( v3.0.0 ) of the puppet-gitlab module.

@LongLiveCHIEF LongLiveCHIEF added this to the v3.0.0 milestone Aug 30, 2018
@gfokkema
Copy link
Contributor Author

Understood, thanks for the clarification.

@gfokkema it's not really a bug. I classify a bug label to be equivalent with a piece of code in this module that doesn't work as intended.

@born4new
Copy link

@LongLiveCHIEF Any rough estimates on when the v3.0.0 will be released?

@LongLiveCHIEF
Copy link
Contributor

LongLiveCHIEF commented Aug 30, 2018

'First week of September I think. the last big bit was getting the puppet-gitlab_ci_runner functionality moved out into it's own module so that the first release of that module could coincide with the removal of those features from this module. That is mostly done, so I just need to make sure testing is tight on both modules and remove any deprecation code and it'll be good to go.

@born4new
Copy link

born4new commented Oct 4, 2018

@LongLiveCHIEF any new ETA for the V3 by any chance?

@LongLiveCHIEF
Copy link
Contributor

@born4new working on it this weekend. Got pulled onto some other stuff and delayed me from working on this. Sorry!

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.

Alertmanager support in the omnibus config
4 participants