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

Fix proxy settings for elasticsearch_plugin.py #9774

Merged
merged 1 commit into from
Feb 20, 2025
Merged

Fix proxy settings for elasticsearch_plugin.py #9774

merged 1 commit into from
Feb 20, 2025
+18 −13

Conversation

timhovius
Copy link
Contributor

SUMMARY

Fixes #9773

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

elasticsearch_plugin

@timhovius timhovius force-pushed the elasticsearch-plugin-fix-proxy-settings branch from d970064 to 436ec1b Compare February 18, 2025 15:54
@ansibullbot
Copy link
Collaborator

cc @ThePixelDeveloper @samdoran
click here for bot help

@ansibullbot ansibullbot added bug This issue/PR relates to a bug module module new_contributor Help guide this first time contributor plugins plugin (any type) labels Feb 18, 2025
@timhovius timhovius force-pushed the elasticsearch-plugin-fix-proxy-settings branch from 436ec1b to 6f91f1b Compare February 18, 2025 16:01
@ansibullbot

This comment was marked as outdated.

Sign in to view
@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Feb 18, 2025
@timhovius timhovius force-pushed the elasticsearch-plugin-fix-proxy-settings branch from 6f91f1b to 5b3d7d3 Compare February 18, 2025 16:13
@ansibullbot ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Feb 18, 2025
@timhovius timhovius force-pushed the elasticsearch-plugin-fix-proxy-settings branch 2 times, most recently from 8315353 to 55682f9 Compare February 18, 2025 16:23
@ansibullbot ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Feb 18, 2025
@timhovius timhovius force-pushed the elasticsearch-plugin-fix-proxy-settings branch from 55682f9 to b7e9c95 Compare February 18, 2025 18:15
russoz
russoz reviewed Feb 18, 2025
View reviewed changes
Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

Hi @timhovius

Thanks for your contribution. I left a comment about reformatting some statements.

Additionally, this module is passing the command line to run_command() as a string rather than a list, which is not recommended.

If I may suggest, take a look at CmdRunner, there is a guide on how to use it in the collection's docs, it might help you with that.

And, it might be interesting to add some testing on this. There's another guide for UTHelper that might come in handy.

@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-9 Automatically create a backport for the stable-9 branch backport-10 Automatically create a backport for the stable-10 branch labels Feb 18, 2025
@timhovius timhovius force-pushed the elasticsearch-plugin-fix-proxy-settings branch 2 times, most recently from b66bb3a to 515e7bb Compare February 19, 2025 12:20
@timhovius
Copy link
Contributor Author

timhovius commented Feb 19, 2025 •
edited
Loading

Hi @timhovius

Thanks for your contribution. I left a comment about reformatting some statements.

Additionally, this module is passing the command line to run_command() as a string rather than a list, which is not recommended.

If I may suggest, take a look at CmdRunner, there is a guide on how to use it in the collection's docs, it might help you with that.

And, it might be interesting to add some testing on this. There's another guide for UTHelper that might come in handy.

Hi, thanks for your response. I changed the command line for both install_plugin and remove_plugin methods to a list instead of a string. Introducing a CmdRunner will be great, but I think this is a bit out of scope for now.

russoz
russoz reviewed Feb 19, 2025
View reviewed changes
Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

Hi, a couple of adjustments on a quick glance, will take another look later today (my TZ)

@timhovius timhovius force-pushed the elasticsearch-plugin-fix-proxy-settings branch from 515e7bb to ee6fcaa Compare February 20, 2025 08:37
@felixfontein felixfontein merged commit ddc1ea6 into ansible-collections:main Feb 20, 2025
138 checks passed
@patchback Patchback
Copy link

patchback bot commented Feb 20, 2025 •
edited
Loading

Backport to stable-9: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-9/ddc1ea6ae4178d8ab0ef7210762febf732efcbb4/pr-9774

Backported as #9785

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Feb 20, 2025
patchback bot pushed a commit that referenced this pull request Feb 20, 2025
@timhovius @patchback
Fix proxy settings for elasticsearch_plugin.py (#9774)
6aa9019 
elasticsearch_plugin: fix error when setting proxy settings

Co-authored-by: Tim Hovius <[email protected]>
(cherry picked from commit ddc1ea6)
@patchback patchback bot mentioned this pull request Feb 20, 2025
Merged
@patchback Patchback
Copy link

patchback bot commented Feb 20, 2025 •
edited
Loading

Backport to stable-10: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-10/ddc1ea6ae4178d8ab0ef7210762febf732efcbb4/pr-9774

Backported as #9786

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Feb 20, 2025
@timhovius @patchback
Fix proxy settings for elasticsearch_plugin.py (#9774)
f341fdd 
elasticsearch_plugin: fix error when setting proxy settings

Co-authored-by: Tim Hovius <[email protected]>
(cherry picked from commit ddc1ea6)
@patchback patchback bot mentioned this pull request Feb 20, 2025
Merged
@felixfontein
Copy link
Collaborator

@timhovius thanks for your contribution!
@russoz @samdoran thanks for reviewing!

felixfontein pushed a commit that referenced this pull request Feb 20, 2025
@patchback @timhovius
[PR #9774/ddc1ea6a backport][stable-10] Fix proxy settings for elasti…
72ea96c 
…csearch_plugin.py (#9786)

Fix proxy settings for elasticsearch_plugin.py (#9774)

elasticsearch_plugin: fix error when setting proxy settings

Co-authored-by: Tim Hovius <[email protected]>
(cherry picked from commit ddc1ea6)

Co-authored-by: Tim Hovius <[email protected]>
felixfontein pushed a commit that referenced this pull request Feb 20, 2025
@patchback @timhovius
[PR #9774/ddc1ea6a backport][stable-9] Fix proxy settings for elastic…
dd63fb5 
…search_plugin.py (#9785)

Fix proxy settings for elasticsearch_plugin.py (#9774)

elasticsearch_plugin: fix error when setting proxy settings

Co-authored-by: Tim Hovius <[email protected]>
(cherry picked from commit ddc1ea6)

Co-authored-by: Tim Hovius <[email protected]>
rt-vnx pushed a commit to rt-vnx/community.general that referenced this pull request Feb 20, 2025
@timhovius @rt-vnx
Fix proxy settings for elasticsearch_plugin.py (ansible-collections#9774
1b22ebe 
)

elasticsearch_plugin: fix error when setting proxy settings

Co-authored-by: Tim Hovius <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@russoz russoz russoz left review comments

@samdoran samdoran samdoran approved these changes

@felixfontein felixfontein felixfontein approved these changes

Assignees
No one assigned
Labels
backport-9 Automatically create a backport for the stable-9 branch backport-10 Automatically create a backport for the stable-10 branch bug This issue/PR relates to a bug module module new_contributor Help guide this first time contributor plugins plugin (any type)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Elasticsearch plugin throws an error when configuring proxy settings
5 participants
@timhovius @ansibullbot @felixfontein @russoz @samdoran