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 a typo in the "matallb_auto_assign" variable name #8949

Merged
merged 2 commits into from
Jun 13, 2022
Merged

fix a typo in the "matallb_auto_assign" variable name #8949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9bf227b
fix a typo in the "matallb_auto_assign" variable name
orange-llajeanne Jun 9, 2022
b120ddb
add metallb check to fail when deprecated "matallb_auto_assign" varia…
orange-llajeanne Jun 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion inventory/sample/group_vars/k8s_cluster/addons.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ metallb_speaker_enabled: true
# metallb_ip_range:
# - "10.5.0.50-10.5.0.99"
# metallb_pool_name: "loadbalanced"
# matallb_auto_assign: true
# metallb_auto_assign: true
# metallb_speaker_nodeselector:
# kubernetes.io/os: "linux"
# metallb_controller_nodeselector:
Expand Down
2 changes: 1 addition & 1 deletion roles/kubernetes-apps/metallb/defaults/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,4 @@ metallb_speaker_tolerations:
operator: Exists
metallb_controller_tolerations: []
metallb_pool_name: "loadbalanced"
matallb_auto_assign: true
metallb_auto_assign: true
Copy link
Contributor

Choose a reason for hiding this comment

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

To support old name in the existing inventory

Suggested change
metallb_auto_assign: true
metallb_auto_assign: >-
{%- if matallb_auto_assign is defined -%}
{{ matallb_auto_assign }}
{%- else -%}
true
{%- endif -%}

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if anyone is actually using the typo'ed variable name. Carrying some compatibility code due to a typo in the past seems like a lot of effort.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this kind of thing is always maintenance burden for developers.
At least, it would be necessary to take 1-2 versions for warning the option is renamed from the original one for users as the deprecation period.
Then we can remove the original one after that.

Copy link
Member

Choose a reason for hiding this comment

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

Well this can be discussed, but a warning in the release note might be enough, or we can add an ansible check and message indeed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated this PR with the solution proposed by @oomichi , but I can still modify it if this discussion ends up identifying another solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

we can add an ansible check and message indeed

@floryut Thanks, that is a good idea for users I think.
@orange-llajeanne Could you update this pull request again based on

kubespray/roles/network_plugin/calico/tasks/check.yml

Lines 2 to 8 in 4726a11

- name: Stop if legacy encapsulation variables are detected (ipip)
assert:
that:
- ipip is not defined
msg: "'ipip' configuration variable is deprecated, please configure your inventory with 'calico_ipip_mode' set to 'Always' or 'CrossSubnet' according to your specific needs"
run_once: True
delegate_to: "{{ groups['kube_control_plane'][0] }}"

as a reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the workaround in defaults/main.yml and added a check in metallb tasks, inspired from other checks from the same file

I think this should be ok now

6 changes: 6 additions & 0 deletions roles/kubernetes-apps/metallb/tasks/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@
- metallb_protocol == 'bgp' and metallb_speaker_enabled
- metallb_peers is not defined or not metallb_peers

- name: Kubernetes Apps | Check that the deprecated 'matallb_auto_assign' variable is not used anymore
fail:
msg: "'matallb_auto_assign' configuration variable is deprecated, please use 'metallb_auto_assign' instead"
when:
- matallb_auto_assign is defined

- name: Kubernetes Apps | Check AppArmor status
command: which apparmor_parser
register: apparmor_status
Expand Down
2 changes: 1 addition & 1 deletion roles/kubernetes-apps/metallb/templates/metallb-config.yml.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ data:
{% for ip_range in metallb_ip_range %}
- {{ ip_range }}
{% endfor %}
{% if matallb_auto_assign == false %}
{% if metallb_auto_assign == false %}
auto-assign: false
{% endif %}
{% if metallb_additional_address_pools is defined %}{% for pool in metallb_additional_address_pools %}
Expand Down