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.

Sign up for GitHub

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

Jump to bottom

Update README.md #830

Merged
merged 5 commits into from
Dec 21, 2023
Merged

Update README.md #830

merged 5 commits into from
Dec 21, 2023

Conversation

xogoodnow
Copy link
Contributor

The following flag has been replaced but it is not mentioned in the main readme file and only in release notes which makes this prone for confusion. --es.cluster_settings has been renamed to --collector.clustersettings

@xogoodnow
Update README.md
aa8defd 
The following flag has been replaced but it is not mentioned in the main readme file and only in release notes which makes this prone for confusion.
--es.cluster_settings has been renamed to --collector.clustersettings



Signed-off-by: Ali <[email protected]>
@clavinjune
Copy link

clavinjune commented Dec 12, 2023 •
edited
Loading

Hi @xogoodnow can we also change this line

elasticsearch_exporter/README.md

Line 91 in e9ad2f2

es.cluster_settings | `cluster` `monitor` |
?

@xogoodnow
Update README.md
78fab46 
Updated "Elasticsearch 7.x security privileges" section on "collector.clustersettings" flag

Signed-off-by: Ali <[email protected]>
@xogoodnow
Copy link
Contributor Author

Hi @clavinjune,
Sure, I updated the file, please let me know if you meant something else.
Cheers

@clavinjune
Copy link

please also remove this one https://github.com/xogoodnow/elasticsearch_exporter/blob/patch-1/README.md?plain=1#L56

@clavinjune
Copy link

@SuperQ @sysadmind could you please help review this MR 🙇

@xogoodnow
Copy link
Contributor Author

xogoodnow commented Dec 15, 2023 •
edited
Loading

Hi @clavinjune, on "es.cluster_settings" (L56), I have mentioned that this flag has been deprecated as of v1.6.0.
So IMHO, I believe it would be better for backwards compatibility to keep this flag in the document.
But if you believe it must be deleted, it would be no problem.

@clavinjune
Copy link

in 1.6.0, if you use that flag, the exporter won't run. Since the said flag is also marked as breaking changes, I don't think we need to have that on the readme, wdyt?

I guess we can leave that to the maintainer's judgement

@xogoodnow
Update README.md
0d6033a 
Omitted "es.cluster_settings" flag.
Also added description so users who are using the older version would not be confused at first glance.

Signed-off-by: Ali <[email protected]>
@xogoodnow
Copy link
Contributor Author

in 1.6.0, if you use that flag, the exporter won't run. Since the said flag is also marked as breaking changes, I don't think we need to have that on the readme, wdyt?

I guess we can leave that to the maintainer's judgement

Yes I agree.
also since no other deprecated flag is mentioned in the readme file, for the sake of consistency I deleted the flag .

As an example in V1.4.0 >> Rename elasticsearch_process_cpu_time_seconds_sum to elasticsearch_process_cpu_seconds_total. But there is no mention of "elasticsearch_process_cpu_time_seconds_sum" in the main readme file (Metrics).

In the next release, the flag --es.snapshots could be renamed to --collector.snapshots. So if that happens, we need to change the readme for this flag as well.
Thank you @clavinjune

@xogoodnow
Update README.md
256dabd 
The "es.cluster_settings" flag must also be omitted from this section "Elasticsearch 7.x security privileges".

Signed-off-by: Ali <[email protected]>
@xogoodnow
Update README.md
6939ab9 
Since the order is alphabetical this flag should come first.
My bad :)

Signed-off-by: Ali <[email protected]>
sysadmind
sysadmind approved these changes Dec 21, 2023
View reviewed changes
Copy link
Contributor

@sysadmind sysadmind left a comment

Choose a reason for hiding this comment

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

LGTM Thanks!

@sysadmind sysadmind merged commit 1a82b98 into prometheus-community:master Dec 21, 2023
4 checks passed
jaimeyh pushed a commit to sysdiglabs/elasticsearch_exporter that referenced this pull request Jun 14, 2024
@xogoodnow @jaimeyh
Update README.md (prometheus-community#830)
e42d365 
* Update README.md

The following flag has been replaced but it is not mentioned in the main readme file and only in release notes which makes this prone for confusion.
--es.cluster_settings has been renamed to --collector.clustersettings



Signed-off-by: Ali <[email protected]>

* Update README.md

Updated "Elasticsearch 7.x security privileges" section on "collector.clustersettings" flag

Signed-off-by: Ali <[email protected]>

* Update README.md

Omitted "es.cluster_settings" flag.
Also added description so users who are using the older version would not be confused at first glance.

Signed-off-by: Ali <[email protected]>

* Update README.md

The "es.cluster_settings" flag must also be omitted from this section "Elasticsearch 7.x security privileges".

Signed-off-by: Ali <[email protected]>

* Update README.md

Since the order is alphabetical this flag should come first.
My bad :)

Signed-off-by: Ali <[email protected]>

---------

Signed-off-by: Ali <[email protected]>
jaimeyh added a commit to sysdiglabs/elasticsearch_exporter that referenced this pull request Jun 14, 2024
@jaimeyh
Revert "Update README.md (prometheus-community#830)"
bc6da74 
This reverts commit e42d365.
jaimeyh added a commit to sysdiglabs/elasticsearch_exporter that referenced this pull request Jun 14, 2024
@jaimeyh
Revert "Update README.md (prometheus-community#830)"
600da20 
This reverts commit e42d365.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sysadmind sysadmind sysadmind approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@xogoodnow @clavinjune @sysadmind