Skip to content
This repository has been archived by the owner on Mar 6, 2023. It is now read-only.

added installation method config to skip install #272

Merged
merged 1 commit into from
Apr 12, 2020
Merged

added installation method config to skip install #272

merged 1 commit into from
Apr 12, 2020

Conversation

DEvil0000
Copy link
Contributor

skipping installation as a option

@github-actions github-actions bot added area/docs Improvements or additions to documentation area/tasks Logic behind ansible role area/vars Ansible variables used in role labels Mar 20, 2020
@paulfantom
Copy link
Member

Installation is already split into install.yml file and we have proper ansible tags in place, so what is the benefit of having this as a variable?

Also this role is intended to be a management tool for a full lifecycle of prometheus application and this includes day 1 and day 2 operations.

@DEvil0000
Copy link
Contributor Author

We do the installation a different way with some other mechanism so we want to skip all installation related things and just configure the software with ansible. This is not too uncommen when looking at other roles.
There however might be other similar cases where you would not want or need the installation to be done e.g. when running the software in a container you would just want the configuration part to be done.

@paulfantom
Copy link
Member

We do the installation a different way

just configure the software with ansible

So you want to literally copy prometheus config file and rules because this is what is left after you disable installation (as you can see in https://github.com/cloudalchemy/ansible-prometheus/blob/master/tasks/configure.yml), in such case why use this role at all? The main benefits of using this role are automated lifecycle of prometheus binary and secure by default systemd service file if you disable those, then everything else can be easily accomplished with one simple execution of copy or template module.

e.g. when running the software in a container

If you run in a container, then after changing configuration file prometheus needs to be reloaded, so you need to reload container. In such case this role doesn't support it and never will, so why going through the hassle of downloading a role which you are using in a way that can be accomplished with simpler methods?

@DEvil0000
Copy link
Contributor Author

Yes, I only need the configure part (and the systemd file part as long as parts of the config are command line parameters) but does it hurt in any way if installation can get skipped?
In my understanding of open source software it is better to improve a existing project and add a (in this case small) feature/improvement then writing about the same solution again.
If you don't want this in your role feel free to close the PR.

@smutel
Copy link

smutel commented Apr 8, 2020

Please consider this feature which is apparently needed by more than one person.

Copy link
Member

@paulfantom paulfantom left a comment

Choose a reason for hiding this comment

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

After some thought let's implement this.

@DEvil0000 Could you rebase?

README.md Outdated
@@ -28,6 +28,7 @@ All variables which can be overridden are stored in [defaults/main.yml](defaults
| Name | Default Value | Description |
| -------------- | ------------- | -----------------------------------|
| `prometheus_version` | 2.16.0 | Prometheus package version. Also accepts `latest` as parameter. Only prometheus 2.x is supported |
| `prometheus_install_method` | "auto" | Prometheus installation method takes `auto` or `manual` where `manual` effectively skips installation. |
Copy link
Member

@paulfantom paulfantom Apr 8, 2020

Choose a reason for hiding this comment

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

Let's switch this to boolen like prometheus_skip_install and set it to false by default.

Also please add a not that this is exclusive with prometheus_binaries_local_dir and prometheus_version

Copy link

Choose a reason for hiding this comment

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

@paulfantom, contact me if @DEvil0000 does not respond to your request

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like I did ;)

Copy link

Choose a reason for hiding this comment

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

It's mean that you will not update this MR anymore ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

excuse me if I was not clear. I updated it and requested a new review before your comment as you can see in its history.
So what do you think is missing?

Copy link

Choose a reason for hiding this comment

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

OK. Thanks.  @paulfantom, is-it ok for you ?

@paulfantom paulfantom merged commit cb28b3e into cloudalchemy:master Apr 12, 2020
@paulfantom
Copy link
Member

Thanks 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/docs Improvements or additions to documentation area/tasks Logic behind ansible role area/vars Ansible variables used in role
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants