split keepalived config and refactoring #203

Kariton · 2022-06-09T20:11:41Z

split task/main.yml in multiple files
use FQCN
use ansible_managed | comment
use loop instead of with_*
split keepalived configs into separated directories / files
keepalived_instances.INSTANCE.state is multiuse (the configuration file state / optional configuration state) -> absent | d(present) / ['MASTER', 'BACKUP']

molecule/default/verify.yml

evrardjp@cd60ab6 credits to magnusbe I replaced ``|length > 0`` with ``is iterable``

@naphta

evrardjp@d59c740 Credits to @naphta I implemented @evrardjp suggested changes.

@jjungnickel

* Add support for ``vrrp_track_file`` and ``track_files`` evrardjp#140 evrardjp#140 Credits to @jjungnickel

evrardjp · 2022-06-13T08:35:03Z

I see a lot of progress, thanks @Kariton !
We might want to refactor this in different PRs, but please keep up the good work :)

evrardjp · 2022-06-13T08:41:32Z

defaults/main.yml

+
+# Remove all keepalived configurations
+# Clears entire {{ keepalived_config_directory_path }}
+keepalived_flush_configuration: False


We should not introduce a variable to flush the configuration, instead, the state of the other variables should help us figure out what to do (the variable contains the full state). If truly impossible to do, then the deployer can just run an ad-hoc ansible command to clean up (and not need an extra variable).

The idea of the previous config was that if you were to change it, you would never have something laying around that cause an issue of misconfiguration.

Kariton · 2022-06-13T10:35:48Z

@evrardjp Thanks for that.

I know that I did a lot of changes and even correction to those.
And sorry for some unexpected mess within other PRs...

I actually think Im done within this overall draft.

In another branch I just started to work on an argument_specs constellation that is sufficient.
But that is a totally different / new thing for me and lightyears away to be another PR.

It is possible to slit this in different PRs.

How should I split this into different PRs?
""resolve pending PRs"

Ensure instance.vips_excluded is not empty #195

Add support for accept/no_accept for instances. #176

Add support for vrrp_track_file and track_files #140

""general code update""

use FQCN

revert FIX "fqcn-builtins Use FQCN for builtin actions." on ansible lint run #199

use ansible_managed | comment

use loop instead of with_*

""feature / function update""

split task/main.yml in multiple files

split keepalived configs into separated directories / files

implement corresponding state handling

keepalived_instances.INSTANCE.state is multiuse (the configuration file state / optional configuration state) -> absent | d(present) / ['MASTER', 'BACKUP']

evrardjp · 2022-06-13T10:56:58Z

Before we go into subdivising this work into body of work that are reviewable separately, I will watch this PR for its concept.

What I can see at first glance:

A) the general code update concept is great. It can be one PR or 3. It definitely needs to be at least 3 commits though: 1 commit for fqcn, 1 commit for revert, 1 commit with loop refactoring. Please let's not forget any change in tox/molecule (ensure the right versions/dependencies).

B) Feature/Function update.
The ansible_managed is linked to how we do templating. I would do that into function update. (it's a new feature to record this).
task/main can indeed be split into multiple files, especially if those are very clear about their scope. I will check readability, as good readability increases contributions.
For the state multiuse, I think I like the concept, but I want to make sure it "feels as a whole" (disabling should change configuration/remove scripts). I need to review this in more details.

C) resolve existing PRs
I don't think we should solve existing PRs inside new PRs, instead we should promote collaboration. But maybe I am idealistic here ;)
I however, would be okay, to move things forward, that we work together on new PRs to fix the existing state. I want to say a big THANK YOU to you for taking that one. Or I should add https://media.giphy.com/media/KzM1lAfJjCWNq/giphy.gif .

Should we start with point C, and then move forward with A and B? What do you prefer? (I feel however that B should come after A)

Kariton · 2022-06-13T12:30:56Z

Thanks for your input and praise. As my changes would impact the overall template and tasks structure I would resolve existing PRs first or second. Later on a rebase would not be feasible. And second (or first) handle 'A' - general code update. And last but not least the feature / function part as it will result in the mentioned new code base. My changes are destructive to some git mechanics like rebase or revert etc. Im glad that you are willing to look into my PR. Greetings, Kariton

…

---------------------------------------- 13.06.2022 12:57:10 Jean-Philippe Evrard ***@***.***>:

Before we go into subdivising this work into body of work that are reviewable separately, I will watch this PR for its concept. What I can see at first glance: A) the general code update concept is great. It can be one PR or 3. It definitely needs to be at least 3 commits though: 1 commit for fqcn, 1 commit for revert, 1 commit with loop refactoring. Please let's not forget any change in tox/molecule (ensure the right versions/dependencies). B) Feature/Function update. The ansible_managed is linked to how we do templating. I would do that into function update. (it's a new feature to record this). task/main can indeed be split into multiple files, especially if those are very clear about their scope. I will check readability, as good readability increases contributions. For the state multiuse, I think I like the concept, but I want to make sure it "feels as a whole" (disabling should change configuration/remove scripts). I need to review this in more details. C) resolve existing PRs I don't think we should solve existing PRs inside new PRs, instead we should promote collaboration. But maybe I am idealistic here ;) I however, would be okay, to move things forward, that we work together on new PRs to fix the existing state. I want to say a big THANK YOU to you for taking that one. Or I should add https://media.giphy.com/media/KzM1lAfJjCWNq/giphy.gif . Should we start with point C, and then move forward with A and B? What do you prefer? (I feel however that B should come after A) — Reply to this email directly, view it on GitHub[#203 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AQCYKFAUKRU7IDGUEJNU4B3VO4HYLANCNFSM5YLIGAPA]. You are receiving this because you were mentioned.[Verfolgungsbild][https://github.com/notifications/beacon/AQCYKFE6PDE7Z5Z7COAR5X3VO4HYLA5CNFSM5YLIGAPKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOITCSUPY.gif]

Kariton · 2022-06-16T10:11:33Z

To continue my work on this I need some guidance.

My thoughts:
Before we can continue in a meaningful way we need to close as many PRs as possible.

As pointed out earlier my changes are destructive to some git mechanics - especially rebase.
This would mean that other pending PRs needs to be resolved prior to my changes as a rebase would be a hassle.
IIRC only the snap PR is somewhat problematic here.

I already altered some IMO useful "no-answer" PRs to help with that.
I might able to also include the snap PR but I know nothing about SNAP.

How I would like to continue:

wait for a "ready" state
- set following PRs on hold
create PR to implement the targeted changes
- split current tasks structure tasks/main.yml
- split current template structure

I don't want to implement the tasks/main.yml split in my already existing PR "general code update" for the already stated reasons...

evrardjp · 2022-07-12T08:15:27Z

Hello,

I agree with you on the splitting, and started to merge some of those great PRs of yours.
I have some questions on those, so let's talk inside each PR.

I would love to keep the current draft PR open, as it gives a few ideas about the future.

Yet, we'll resolve in the different PRs :)

Kariton · 2022-07-14T22:46:41Z

solved and not needed anymore as i found another solution which is sufficent:
#200 (comment)

split keepalived config and refactoring

ba46e01

Kariton mentioned this pull request Jun 9, 2022

Add more execution granularity / idempotency #200

Closed

Kariton commented Jun 9, 2022

View reviewed changes

molecule/default/verify.yml Outdated Show resolved Hide resolved

Kariton added 11 commits June 9, 2022 23:38

Ensure instance.vips_excluded is not empty evrardjp#195

c3a308a

evrardjp@cd60ab6 credits to magnusbe I replaced ``|length > 0`` with ``is iterable``

Add support for accept/no_accept for instances. evrardjp#176

2079e31

evrardjp@d59c740 Credits to @naphta I implemented @evrardjp suggested changes.

fix accept/no_accept for instances missing tag

1cf89c6

fix some stuff...

bcb871f

fix accept/no_accept for instances missing tag II

359a456

Add support for vrrp_track_file and track_files evrardjp#140 (#2)

f437e89

* Add support for ``vrrp_track_file`` and ``track_files`` evrardjp#140 evrardjp#140 Credits to @jjungnickel

fix hardcoded filepath

1d30d9b

remove fqcn-builtins exclude from ansible-lint from evrardjp#199

88369ea

add ability to flush_configuration

2512370

replace my vips_excluded is iterable with length

f10bd8e

FQCN ansible.netcommon.ipaddr

5a05ff2

evrardjp reviewed Jun 13, 2022

View reviewed changes

This was referenced Jun 16, 2022

Add new features vrrp_track_file, track_files, (no) accept, and safeguards. #204

Merged

General ansible practices refresh #205

Merged

Kariton mentioned this pull request Jul 12, 2022

Add more execution granularity / idempotency by splitting configuration #209

Closed

Kariton closed this Jul 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

split keepalived config and refactoring #203

split keepalived config and refactoring #203

Kariton commented Jun 9, 2022

evrardjp commented Jun 13, 2022 •

edited

Loading

evrardjp Jun 13, 2022

Kariton commented Jun 13, 2022

evrardjp commented Jun 13, 2022

Kariton commented Jun 13, 2022 via email •

edited

Loading

Kariton commented Jun 16, 2022

evrardjp commented Jul 12, 2022

Kariton commented Jul 14, 2022

split keepalived config and refactoring #203

split keepalived config and refactoring #203

Conversation

Kariton commented Jun 9, 2022

evrardjp commented Jun 13, 2022 • edited Loading

evrardjp Jun 13, 2022

Choose a reason for hiding this comment

Kariton commented Jun 13, 2022

evrardjp commented Jun 13, 2022

Kariton commented Jun 13, 2022 via email • edited Loading

Kariton commented Jun 16, 2022

evrardjp commented Jul 12, 2022

Kariton commented Jul 14, 2022

evrardjp commented Jun 13, 2022 •

edited

Loading

Kariton commented Jun 13, 2022 via email •

edited

Loading