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

Update options flag for nginx-prometheus-exporter to new double dash format #816

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lbdemv
Copy link

@lbdemv lbdemv commented Oct 24, 2024

Pull Request (PR) description

The nginx exporter logs the following deprecation warning after startup.

nginx-prometheus-exporter: the flag format is deprecated and will be removed in a future release, please use the new format: --nginx.scrape-uri

This PR intends to fix the deprecated flag format and use the current flag format with double dashes.

This Pull Request (PR) fixes the following issues

No issue opened so far.

@TheMeier
Copy link
Collaborator

@lbdemv do you know since when the "--" versions are supported?
https://github.com/nginxinc/nginx-prometheus-exporter/releases/tag/v1.0.0 ?

@lbdemv
Copy link
Author

lbdemv commented Oct 24, 2024

Sorry, I didn't actually check that. I just saw it and went on to fix the current warning...

Maybe I can dwell into that later today, if it is important.

@lbdemv
Copy link
Author

lbdemv commented Oct 24, 2024

As i see that, the deprecation change is made in nginx/nginx-prometheus-exporter#460
It was a fix for nginx/nginx-prometheus-exporter#447

The initial issue seem to be the change to kigpin in nginx/nginx-prometheus-exporter#420 which was flagged as a breaking change in the mentioned v1.0.0 changelog

@TheMeier
Copy link
Collaborator

I see. The default version in this module is currently 0.11.0 (https://github.com/voxpupuli/puppet-prometheus/blob/master/manifests/nginx_prometheus_exporter.pp#L66) so we need a version comparision here. Also there is a pending MR (#783) to update the version but that needs some work :(

Copy link
Member

@tuxmea tuxmea left a comment

Choose a reason for hiding this comment

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

We still need the version comparison.

@TheMeier TheMeier modified the milestone: v16.0.0 Dec 30, 2024
@TheMeier TheMeier changed the title Update options flag to new double dash format Update options flag for nginx-prometheus-exporter to new double dash format Dec 30, 2024
@TheMeier TheMeier added the enhancement New feature or request label Dec 30, 2024
@lbdemv
Copy link
Author

lbdemv commented Jan 8, 2025

@tuxmea I'm not sure if you are waiting for me to add anything? Or was this comment more like a memo for yourself?

@tuxmea
Copy link
Member

tuxmea commented Jan 8, 2025

memo

@TheMeier
Copy link
Collaborator

@lbdemv we need something like this:

if versioncmp($version, '1.0.0') >= 0 {
  $options = "--nginx.scrape-uri '${scrape_uri}' ${extra_options}"
} else {
  $options = "-nginx.scrape-uri '${scrape_uri}' ${extra_options}"
}

The tests need to be changed to check both cases. If you don't mind I could update your PR accordingly.

The nginx exporter logs the following deprecation warning after startup.

```
nginx-prometheus-exporter: the flag format is deprecated and will be removed in a future release, please use the new format: --nginx.scrape-uri
```

This PR intends to fix that deprecated format and use the current flag format with double dashes.
@lbdemv
Copy link
Author

lbdemv commented Jan 12, 2025

I don't mind at all. Go right ahead 😄

@TheMeier TheMeier added this to the v16.1.0 milestone Jan 12, 2025
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.

3 participants