Refactor of consul modules #7826

apollo13 · 2024-01-12T19:15:22Z

SUMMARY

Refactors common code from consul_modules into module_utils

ISSUE TYPE

Refactoring Pull Request

COMPONENT NAME

consul

ansibullbot · 2024-01-12T19:35:56Z

cc @sgargan
click here for bot help

apollo13 · 2024-01-12T19:53:12Z

Hopefully fixed the ansible-test issues. I noticed that consul_acl doesn't have any tests, so before merging this I should probably add those as well to ensure the refactor didn't break anything.

felixfontein

Thanks for your contribution!

Two first comments:

plugins/modules/consul.py

felixfontein · 2024-01-12T20:14:33Z

plugins/doc_fragments/consul.py

+    description:
+      - The protocol scheme on which the consul agent is running.
+    default: http
+    choices: [ http, https ]


Adding the choices here is a breaking change. (Or was it absolutely impossible to specify anything else than http and https as the scheme before?)

I wouldn't know what else one could specify. But I'll happily remove it if you think it is to dangerous.

I guess it's better to not have choices here then. You could mention both choices in the description if you want, if you want to make sure users know what they can specify here (that the second choices is https is probably not completely obvious to everyone).

It is a weird setting anyways, in the long run it would probably make more sense to deprecate this and have a simple ssl on/off toggle.

If this change remains, this PR cannot be merged because it contains a breaking change.

It uses deprecated APIs disabled since Consul 1.11 (which is EOL), don't bother updating the module anymore.

apollo13 · 2024-01-13T18:42:37Z

@felixfontein I think this is review ready. The rewrite does not cover consul_kv or consul_acl. The latter doesn't work in recent consul anymore and should be dropped in favor of a new consul_token in addition to the existing consul_policy. This is because it uses the legacy API: https://developer.hashicorp.com/consul/api-docs/v1.14.x/acl/legacy

As for consul_kv -- it would require a full rewrite maybe without the python-consul module which hasn't seen a release since 2018 or so.

While on it I dropped the dependency on requests as well, not really much of a reason to require it.

For me this is basically foundational work to be able to add binding rules and acl auth methods via later commits. But I'd love to get this reviewed and merged before because it improves stuff independently.

felixfontein

I haven't yet checked the module code changes.

Please add a changelog fragment.

plugins/module_utils/consul.py

plugins/modules/consul_session.py

apollo13 · 2024-01-17T06:59:16Z

Thank you for the review. I will fix it over the course of the week. I might have more questions so I know what to look for and where next time and you don't have to tell me to fix obvious stuff then... 😄

…

On Tue, Jan 16, 2024, at 22:37, Felix Fontein wrote: ***@***.**** commented on this pull request. I haven't yet checked the module code changes. Please add a changelog fragment <https://docs.ansible.com/ansible/devel/community/collection_development_process.html#creating-a-changelog-fragment>. In plugins/module_utils/consul.py <#7826 (comment)>: > @@ -5,25 +5,93 @@ # SPDX-License-Identifier: GPL-3.0-or-later from __future__ import absolute_import, division, print_function -__metaclass__ = type Please keep `__metaclass = type` next to `from __future__ import ...`. In plugins/module_utils/consul.py <#7826 (comment)>: > -def get_consul_url(configuration): This function is part of the public API of the collection and needs to stay until the next major release. (Same for `get_auth_headers` and `handle_consul_response_error` below.) In plugins/modules/consul_session.py <#7826 (comment)>: > - token: - description: - - The token key identifying an ACL rule set that controls access to - the key value pair. - type: str - version_added: 5.6.0 ` token: version_added: 5.6.0 ` needs to stay. — Reply to this email directly, view it on GitHub <#7826 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAT5C2QDLKNTR3PLFJXX3TYO3XLHAVCNFSM6AAAAABBYS32FSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQMRVGE3TKOBXGA>. You are receiving this because you authored the thread.Message ID: ***@***.***>

apollo13 · 2024-01-17T16:09:24Z

I think I managed to address all comments, let me know if I missed something, thanks!

felixfontein

Looks good to me. Do you want to do more changes, or should I merge this?

apollo13 · 2024-01-21T14:08:52Z

As far as the refactoring goes, I think I am done with it. If possible I would like to mark ConsulModule private for now till it's API is stable -- for that I would move it into _consul.py and mark document it as private?

felixfontein · 2024-01-21T14:17:53Z

I would simply rename ConsulModule to _ConsulModule and add a comment that it is a private API for now and breaking changes can appear at any time, even in bugfix releases.

That should be clear enough :)

apollo13 · 2024-01-21T16:09:56Z

Okay, I have marked it private and added a note to the doc string. Let me know if there is anything else needed. I will be working on the new modules now.

felixfontein · 2024-01-21T17:29:18Z

plugins/modules/consul_policy.py

@@ -135,22 +103,11 @@
 """

 from ansible.module_utils.basic import AnsibleModule
+from ansible_collections.community.general.plugins.module_utils.consul import (
+    _ConsulModule, auth_argument_spec)


Here you could also do

Suggested change

_ConsulModule, auth_argument_spec)

_ConsulModule as ConsulModule, auth_argument_spec)

to avoid propagating the rename to the modules, but let's keep it at is now :)

patchback · 2024-01-21T17:29:33Z

Backport to stable-8: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-8/44679e71a23c37bfe9a047bd4243666d4b12c459/pr-7826

Backported as #7877

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

* Extract common functionality. * Refactor duplicated code into module_utils. * Fixed ansible-test issues. * Address review comments. * Revert changes to consul_acl. It uses deprecated APIs disabled since Consul 1.11 (which is EOL), don't bother updating the module anymore. * Remove unused code. * Merge token into default doc fragment. * JSON all the way down. * extract validation tests into custom file and prep for requests removal. * Removed dependency on requests. * Initial test for consul_kv. * fixup license headers. * Revert changes to consul.py since it utilizes python-consul. * Disable the lookup test for now. * Fix python 2.7 support. * Address review comments. * Address review comments. * Addec changelog fragment. * Mark ConsulModule as private. (cherry picked from commit 44679e7)

felixfontein · 2024-01-21T17:29:42Z

@apollo13 thanks a lot for your contribution!

) Refactor of consul modules (#7826) * Extract common functionality. * Refactor duplicated code into module_utils. * Fixed ansible-test issues. * Address review comments. * Revert changes to consul_acl. It uses deprecated APIs disabled since Consul 1.11 (which is EOL), don't bother updating the module anymore. * Remove unused code. * Merge token into default doc fragment. * JSON all the way down. * extract validation tests into custom file and prep for requests removal. * Removed dependency on requests. * Initial test for consul_kv. * fixup license headers. * Revert changes to consul.py since it utilizes python-consul. * Disable the lookup test for now. * Fix python 2.7 support. * Address review comments. * Address review comments. * Addec changelog fragment. * Mark ConsulModule as private. (cherry picked from commit 44679e7) Co-authored-by: Florian Apolloner <[email protected]>

apollo13 added 2 commits January 12, 2024 19:41

Extract common functionality.

515baff

Refactor duplicated code into module_utils.

7402b03

This comment was marked as outdated.

Sign in to view

ansibullbot added docs_fragments docs_fragments plugin (shared docs) module module module_utils module_utils needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR plugins plugin (any type) labels Jan 12, 2024

This comment was marked as outdated.

Sign in to view

apollo13 force-pushed the consul-refactor branch from c9b9ad5 to 580f9a9 Compare January 12, 2024 20:03

felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-8 Automatically create a backport for the stable-8 branch labels Jan 12, 2024

apollo13 force-pushed the consul-refactor branch from 580f9a9 to 1260acc Compare January 12, 2024 20:09

felixfontein reviewed Jan 12, 2024

View reviewed changes

Fixed ansible-test issues.

ebebb13

apollo13 force-pushed the consul-refactor branch from 1260acc to ebebb13 Compare January 12, 2024 20:16

ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Jan 12, 2024

apollo13 added 8 commits January 13, 2024 13:20

Address review comments.

b9368b0

Revert changes to consul_acl.

72a090c

It uses deprecated APIs disabled since Consul 1.11 (which is EOL), don't bother updating the module anymore.

Remove unused code.

c3b3f42

Merge token into default doc fragment.

1b7afc3

JSON all the way down.

95bee24

extract validation tests into custom file and prep for requests removal.

02dec8a

Removed dependency on requests.

1e77836

Initial test for consul_kv.

53617e2

ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jan 13, 2024

fixup license headers.

0e498f9

ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Jan 13, 2024

Address review comments.

346bbdc

felixfontein reviewed Jan 16, 2024

View reviewed changes

plugins/module_utils/consul.py Show resolved Hide resolved

plugins/module_utils/consul.py Show resolved Hide resolved

plugins/modules/consul_session.py Outdated Show resolved Hide resolved

apollo13 force-pushed the consul-refactor branch 3 times, most recently from 854c2c8 to 0d738bd Compare January 17, 2024 16:07

ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jan 17, 2024

apollo13 added 2 commits January 17, 2024 17:13

Address review comments.

0821bb6

Addec changelog fragment.

33ad1d8

apollo13 force-pushed the consul-refactor branch from 0d738bd to 33ad1d8 Compare January 17, 2024 16:13

ansibullbot removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jan 17, 2024

felixfontein approved these changes Jan 21, 2024

View reviewed changes

Mark ConsulModule as private.

0421fca

apollo13 force-pushed the consul-refactor branch from d3c04ad to 0421fca Compare January 21, 2024 16:09

felixfontein reviewed Jan 21, 2024

View reviewed changes

felixfontein merged commit 44679e7 into ansible-collections:main Jan 21, 2024
120 of 121 checks passed

patchback bot mentioned this pull request Jan 21, 2024

[PR #7826/44679e71 backport][stable-8] Refactor of consul modules #7877

Merged

felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Jan 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor of consul modules #7826

Refactor of consul modules #7826

apollo13 commented Jan 12, 2024

This comment was marked as outdated.

ansibullbot commented Jan 12, 2024

apollo13 commented Jan 12, 2024

This comment was marked as outdated.

felixfontein left a comment

felixfontein Jan 12, 2024

apollo13 Jan 12, 2024

felixfontein Jan 13, 2024

apollo13 Jan 14, 2024

felixfontein Jan 16, 2024

apollo13 commented Jan 13, 2024

felixfontein left a comment

apollo13 commented Jan 17, 2024 via email

apollo13 commented Jan 17, 2024

felixfontein left a comment

apollo13 commented Jan 21, 2024

felixfontein commented Jan 21, 2024

apollo13 commented Jan 21, 2024

felixfontein Jan 21, 2024

patchback bot commented Jan 21, 2024 •

edited

Loading

felixfontein commented Jan 21, 2024

	_ConsulModule, auth_argument_spec)
	_ConsulModule as ConsulModule, auth_argument_spec)

Refactor of consul modules #7826

Refactor of consul modules #7826

Conversation

apollo13 commented Jan 12, 2024

SUMMARY

ISSUE TYPE

COMPONENT NAME

This comment was marked as outdated.

ansibullbot commented Jan 12, 2024

apollo13 commented Jan 12, 2024

This comment was marked as outdated.

felixfontein left a comment

Choose a reason for hiding this comment

felixfontein Jan 12, 2024

Choose a reason for hiding this comment

apollo13 Jan 12, 2024

Choose a reason for hiding this comment

felixfontein Jan 13, 2024

Choose a reason for hiding this comment

apollo13 Jan 14, 2024

Choose a reason for hiding this comment

felixfontein Jan 16, 2024

Choose a reason for hiding this comment

apollo13 commented Jan 13, 2024

felixfontein left a comment

Choose a reason for hiding this comment

apollo13 commented Jan 17, 2024 via email

apollo13 commented Jan 17, 2024

felixfontein left a comment

Choose a reason for hiding this comment

apollo13 commented Jan 21, 2024

felixfontein commented Jan 21, 2024

apollo13 commented Jan 21, 2024

felixfontein Jan 21, 2024

Choose a reason for hiding this comment

patchback bot commented Jan 21, 2024 • edited Loading

Backport to stable-8: 💚 backport PR created

felixfontein commented Jan 21, 2024

patchback bot commented Jan 21, 2024 •

edited

Loading