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.

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

add possibility to manage network interfaces with a template #497

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

trefzer
Copy link
Contributor

@trefzer trefzer commented Nov 12, 2024

This gives the possibility to manage all network/netdev/link settings from hashes.

I'm aware of pull request #340 from @rwaffen which started an equal solution last year.
Since the development there does not seem to go on, I started this solution which currrently
gives us the opportunitiy to create any configuration stated in the systemd documentation.

All type defintions are taken out of systemd documentation (not documented, not in our type defintion).
I'm aware, that some types could defined more accurate, but I tried to get a compromise inbetween accurate type and
workload ;).

Comment on lines +85 to +87
fname => "${_filename}.link",
config => deep_merge(pick($link_profiles[$interface_name], {}), $interface['link']),
}),
Copy link
Member

Choose a reason for hiding this comment

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

Better to put the closing braces on separate lines to avoid the need for double indentation:

Suggested change
fname => "${_filename}.link",
config => deep_merge(pick($link_profiles[$interface_name], {}), $interface['link']),
}),
fname => "${_filename}.link",
config => deep_merge(pick($link_profiles[$interface_name], {}), $interface['link']),
},
),

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 don't think that this makes the code better readable. I will not change that for now.

Copy link
Member

Choose a reason for hiding this comment

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

It does. The double indentation that you have to do currently is really just a bug in puppet-lint, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no the double intentation is correct since there are to brackets to intent.
to clarify:
my solution:

epp('template, {
    var => val,
})

your solution:

epp('template,
 {
    var => val,
 }
)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, IMO it is easier to follow the indentation when there is only at most one level of change between each line (your example has var => val, indented one level too far in my solution), easier to match up the opening and closing braces, and reduces diffs when adding and removing items to the comma-separated lists by already having separate lines.

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 made a test commit with your suggestion. I expect it to fail on validation test ! let's see

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok validatio now fails, the it needs 12 spaces, no way to lower that.
I will remove the last commit tomorrow. Think we can resolv this.

Copy link
Member

Choose a reason for hiding this comment

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

Right, it's clearly not correct to have the closing brace on the same line as the content inside of the brace. You'd have to also move the opening brace and fix the indentation.

manifests/networkd.pp Outdated Show resolved Hide resolved
manifests/networkd.pp Outdated Show resolved Hide resolved
manifests/networkd.pp Outdated Show resolved Hide resolved
manifests/networkd.pp Outdated Show resolved Hide resolved
manifests/networkd.pp Show resolved Hide resolved
manifests/networkd.pp Outdated Show resolved Hide resolved
trefzer and others added 4 commits November 12, 2024 20:14
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.

2 participants