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

fix: scrape config files path #377

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

manzsolutions-lpr
Copy link
Contributor

@manzsolutions-lpr manzsolutions-lpr commented Jun 3, 2024

Unfortunately simply rebasing the original PR I missed that the paths in the actual role are wrong. I have now updated them and took the chance to also re-use the variable's value for the actual configuration not just the glob for copying.

//edit: Cannot reuse variable since it would lead to /etc/prometheus/prometheus/scrapes/....

@manzsolutions-lpr manzsolutions-lpr changed the title Fix scrape config files path Draft: Fix scrape config files path Jun 3, 2024
@manzsolutions-lpr manzsolutions-lpr marked this pull request as draft June 3, 2024 14:47
@manzsolutions-lpr manzsolutions-lpr changed the title Draft: Fix scrape config files path Fix scrape config files path Jun 3, 2024
@manzsolutions-lpr manzsolutions-lpr marked this pull request as ready for review June 3, 2024 15:09
@manzsolutions-lpr
Copy link
Contributor Author

@gardar I would also add "notify"-Statements to all the file uploads to trigger a reload of Prometheus. May I add this in this PR?

@gardar gardar changed the title Fix scrape config files path fix: scrape config files path Jun 6, 2024
Copy link
Member

@gardar gardar left a comment

Choose a reason for hiding this comment

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

@gardar I would also add "notify"-Statements to all the file uploads to trigger a reload of Prometheus. May I add this in this PR?

Sure, go ahead.
Could you also please add tests to molecule molecule so that we can verify that it actually works as intended?

@gardar gardar requested a review from SuperQ September 16, 2024 11:57
@manzsolutions-lpr
Copy link
Contributor Author

@SuperQ Is there anything I can do to make the review easier for you?

@gardar
Copy link
Member

gardar commented Sep 30, 2024

Can you please rebase and fix the conflicts with the main branch?

Copy link
Contributor

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

I think we should name this more explicitly the same to make it clear what it is.

roles/prometheus/defaults/main.yml Outdated Show resolved Hide resolved
@github-actions github-actions bot added bugfix and removed bugfix labels Oct 4, 2024
Signed-off-by: Leonhard Mayr <leonhard.mayr@manz.at>
Signed-off-by: Leonhard Mayr <leonhard.mayr@manz.at>
Signed-off-by: Leonhard Mayr <leonhard.mayr@manz.at>
Signed-off-by: Leonhard Mayr <leonhard.mayr@manz.at>
Copy link
Contributor

github-actions bot commented Oct 4, 2024

Docs Build 📝

Thank you for contribution!✨

The docs for this PR have been published here:
https://prometheus-community.github.io/ansible/pr/377

You can compare to the docs for the main branch here:
https://prometheus-community.github.io/ansible/branch/main

The docsite for this PR is also available for download as an artifact from this run:
https://github.com/prometheus-community/ansible/actions/runs/11175404475

File changes:

Click to see the diff comparison.

NOTE: only file modifications are shown here. New and deleted files are excluded.
See the file list and check the published docs to see those files.

diff --git a/home/runner/work/ansible/ansible/docsbuild/base/prometheus_role.html b/home/runner/work/ansible/ansible/docsbuild/head/prometheus_role.html
index b3a5554..8b26d8e 100644
--- a/home/runner/work/ansible/ansible/docsbuild/base/prometheus_role.html
+++ b/home/runner/work/ansible/ansible/docsbuild/head/prometheus_role.html
@@ -335,9 +335,9 @@ To check whether it is installed, run <code class="code docutils literal notrans
 <div class="ansibleOptionAnchor" id="parameter-main--prometheus_scrape_config_files"></div><p class="ansible-option-title" id="ansible-collections-prometheus-prometheus-prometheus-role-parameter-main-prometheus-scrape-config-files"><strong>prometheus_scrape_config_files</strong></p>
 <a class="ansibleOptionLink" href="#parameter-main--prometheus_scrape_config_files" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">list</span> / <span class="ansible-option-elements">elements=string</span></p>
 </div></td>
-<td><div class="ansible-option-cell"><p>List of folders where ansible will look for files containing custom scrape config configuration files which will be copied to <code class="docutils literal notranslate"><span class="pre">{{</span> <span class="pre">prometheus_config_dir</span> <span class="pre">}}/scrapes/</span></code>.</p>
+<td><div class="ansible-option-cell"><p>List of folders where ansible will look for files containing custom scrape config configuration files which will be copied to <code class="docutils literal notranslate"><span class="pre">{{</span> <span class="pre">prometheus_config_dir</span> <span class="pre">}}/scrape_configs/</span></code>.</p>
 <p>This feature is available starting from Prometheus v2.43.0.</p>
-<p class="ansible-option-line"><strong class="ansible-option-default-bold">Default:</strong> <code class="ansible-option-default docutils literal notranslate"><span class="pre">[&quot;prometheus/scrapes/*.yml&quot;,</span> <span class="pre">&quot;prometheus/scrapes/*.json&quot;]</span></code></p>
+<p class="ansible-option-line"><strong class="ansible-option-default-bold">Default:</strong> <code class="ansible-option-default docutils literal notranslate"><span class="pre">[&quot;prometheus/scrape_configs/*.yml&quot;,</span> <span class="pre">&quot;prometheus/scrape_configs/*.json&quot;]</span></code></p>
 </div></td>
 </tr>
 <tr class="row-even"><td><div class="ansible-option-cell">

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants