Skip to content

Conversation

@pilem
Copy link

@pilem pilem commented Mar 7, 2025

Restarting Network Bonds added
Conditions to configure defined interfaces added
source /etc/network/interfaces.d/* added

@btravouillon btravouillon self-requested a review March 10, 2025 15:02
Copy link

@btravouillon btravouillon left a comment

Choose a reason for hiding this comment

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

So sorry I missed this one when we were working together @pilem. 😕

tasks/main.yml Outdated
item['configure'] and
config_interface['changed']

- name: Restarting Network Bonds

Choose a reason for hiding this comment

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

Should this restart before the bridges?
I guess there is a highest chance that a bridge requires a bond to exist than the opposite.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, good catch. I'll swap the restart order.

tasks/main.yml Outdated
Comment on lines 57 to 60
when: (ifupdown_network_interfaces | length > 0 or ifupdown_network_vlans | length > 0 or
ifupdown_network_bridges | length > 0 or ifupdown_network_bonds | length > 0 or
ifupdown_ovs_bridges | length > 0 or ifupdown_ovs_bonds | length > 0 or
ifupdown_ovs_interfaces | length > 0 or ifupdown_ovs_ports | length > 0)

Choose a reason for hiding this comment

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

I wonder if this when clause makes sense. I would remove it and execute this task at each run.

I mean, I can't see no obvious reason to use this role if not to configure /etc/network/interfaces. 🤔

Copy link
Author

Choose a reason for hiding this comment

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

That makes sense. I'll remove this condition.

dns-search {{ ifupdown_dns_search }}
{% endif %}

source /etc/network/interfaces.d/*

Choose a reason for hiding this comment

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

Could you please make it a condition to not change the default behaviour?

Suggested change
source /etc/network/interfaces.d/*
{% if ifupdown_source_interfaces_d | bool %}
source /etc/network/interfaces.d/*
{% endif %}
  • add this to defaults/main.yml:
ifupdown_source_interfaces_d: false

Copy link
Author

Choose a reason for hiding this comment

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

Sure, not a problem.

…art, made sourcing interfaces.d content optional
Copy link

@btravouillon btravouillon left a comment

Choose a reason for hiding this comment

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

Looks good to me. 👍

ping @pbouchez 😺

@btravouillon
Copy link

fwiw 2 workflows are awaiting approval from a maintainer (which I'm not anymore 😢)

@pbouchez
Copy link

fwiw 2 workflows are awaiting approval from a maintainer (which I'm not anymore 😢)

I ran them and the checks are now failing :(

@btravouillon
Copy link

@pbouchez #26 for ansible-lint.

@btravouillon
Copy link

And #27 for molecule tests.

@pbouchez
Copy link

@pilem :

Merging is blocked
Commits must have verified signatures.

Do you have a working GPG key associated to your GitHub account?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants