Fixes / extends configuration of the positions file #39
Conversation
tasks/install.yml
Outdated
| - name: Create config and data directories | ||
| file: | ||
| path: "{{ item }}" | ||
| state: directory | ||
| owner: root | ||
| group: "{{ promtail_system_group }}" | ||
| mode: 0770 | ||
| with_items: | ||
| loop: | ||
| - "{{ promtail_config_dir }}" | ||
| - "{{ promtail_config_file_sd_dir }}" | ||
| - "{{ promtail_positions_path | dirname }}" |
There was a problem hiding this comment.
The proposed change would modify any existing directory - i.e. promtail_positions_path: /tmp/positions.yml.
I currently use promtail_config_positions to specify the filename directly :
promtail_config_positions:
filename: /tmp/positions.yaml
This would not pick up the specified filename and still create a unnecessary directory.
Instead of creating a directory - I propose to rather create the file directly (promtail_positions_path .filename) with correct permissions for loki
There was a problem hiding this comment.
hm, i can't come up with something else than to create the directory conditionally when non-existing to cover both cases - using a common or a distinct directory.
|
Thank you very much for submitting the PR I was working on moving the CI over to github actions - would it be possible that you rebase your work and push it freshly so we have a clean CI run? |
funkyfuture
force-pushed
the
positions
branch
from
29da756
to
09b6a77
Compare
funkyfuture
force-pushed
the
positions
branch
from
09b6a77
to
c79c08e
Compare
|
a'ight, i added the changes w/o squashing commits. |
defaults/main.yml
Outdated
| # filename: /tmp/positions.yaml | ||
| promtail_positions_path: /var/lib/promtail/positions.yml | ||
| promtail_config_positions: | ||
| filename: "{{ promtail_positions_path }}" |
There was a problem hiding this comment.
| filename: "{{ promtail_positions_path }}" | |
| filename: "{{ promtail_positions_path }}/positions.yml" |
defaults/main.yml
Outdated
| promtail_config_positions: {} | ||
| # promtail_config_positions: | ||
| # filename: /tmp/positions.yaml | ||
| promtail_positions_path: /var/lib/promtail/positions.yml |
There was a problem hiding this comment.
| promtail_positions_path: /var/lib/promtail/positions.yml | |
| promtail_positions_path: /var/lib/promtail |
tasks/install.yml
Outdated
| state: directory | ||
| owner: root | ||
| group: "{{ promtail_system_group }}" | ||
| mode: ug=rwx,o= |
There was a problem hiding this comment.
| mode: ug=rwx,o= | |
| mode: 0770 |
Let's please use the same syntax for all tasks. I am open to adjusting all tasks with a different syntax if there is any benefit to it 👍
|
Thanks for looking further into this - highly appreciated! After having given this some thought - what about the following suggestion: The path variable only defines the path that is managed by this role (see the suggested change) and we document that this path will be created/managed by the role. That way do not need to conditionally create something - but rather inform the user of the role how it shall be used. |
i'm fine with that. am i getting correctly by your suggested changes that userscould only change the containing directory, but not the filename (unless using |
Exactly - that was my thought. Thanks for picking up on that and please excuse for not being explicit. |
|
a'ight, i amended accordingly. |
|
unfortunately, i can't see what is causing the ci failure. |
|
The failure is strange - taking a look |
|
#44 should fix the failing CI - will re-run the pipelines after it is merged and then review the PR |
here's a proposal for handling the positions file configuration.