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 bond interfaces supporting #37

Open
wants to merge 2 commits into
base: devel
Choose a base branch
from

Conversation

ObjatieGroba
Copy link
Collaborator

@ObjatieGroba ObjatieGroba commented Mar 9, 2021

  • Reformat keys with lower case. Replace '_' with '-'.
  • Support multiword values on parsing
  • Bonding support
  • Add bond attributes checking
  • Add bond settings validation
  • Add bond configuration tests

@ObjatieGroba ObjatieGroba force-pushed the feature_bond_interfaces branch 3 times, most recently from a6725df to 03bc049 Compare March 9, 2021 17:40
sline = [x.strip() for x in line.split()]
sline = [x.strip() for x in line.split(None, 1)]

sline[0] = sline[0].lower().replace('_', '-')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are no standard way to write such attributes.

It can be lower or upper case.

'-' and '_' are equal in this context.

Suggest to use lower-case for keys and some attribute values

@ObjatieGroba
Copy link
Collaborator Author

Let's release 3.5.0 (merge devel to main) and than refactor it as 3.6.0, do we?

@ObjatieGroba ObjatieGroba added this to the v3.6.0 milestone Mar 14, 2021
Copy link
Owner

@nMustaki nMustaki left a comment

Choose a reason for hiding this comment

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

i know nothing about bonds, I trust you on the business logic !


return self.addAdapter({"name": name, 'bond-mode': mode, 'bond-slaves': ' '.join(slaves)})

def validateBondSettings(self):
Copy link
Owner

Choose a reason for hiding this comment

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

I would move that method into NetworkAdapterValidation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Move to the new File infaterfacesValidation to class InterfacesValidator

key (str): the option name
val (any): the option value
"""
key = key.lower().replace('_', '-')
Copy link
Owner

Choose a reason for hiding this comment

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

Are you sure about that one ? It may break a keyword ?

Copy link
Collaborator Author

@ObjatieGroba ObjatieGroba Mar 22, 2021

Choose a reason for hiding this comment

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

It should not break keyword. As NetworkManager sources says:

Normalize keys. Convert '_' to '-', as ifupdown accepts both variants.

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/blob/master/src/core/settings/plugins/ifupdown/nms-ifupdown-interface-parser.c#L76

Copy link
Collaborator Author

@ObjatieGroba ObjatieGroba Mar 22, 2021

Choose a reason for hiding this comment

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

Sources of ifupdown cast all attr names into UPPER_SNAKE_CASE and accept any variants - lower or upper, '-' or '_' before writing it into OS

https://salsa.debian.org/debian/ifupdown/-/blob/master/execute.c#L41

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

@ObjatieGroba ObjatieGroba Mar 22, 2021

Choose a reason for hiding this comment

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

But there are some options that should be this-case and neither another cases:

https://salsa.debian.org/debian/ifupdown/-/blob/master/config.c#L733

That's why I suggest to do such transformation

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the explanations !

@ObjatieGroba ObjatieGroba changed the title Add bond interfaces supporting Draft: Add bond interfaces supporting Mar 22, 2021
@ObjatieGroba ObjatieGroba force-pushed the feature_bond_interfaces branch from 6fc49bf to 99bbe55 Compare June 22, 2021 10:09
@ObjatieGroba ObjatieGroba changed the title Draft: Add bond interfaces supporting Add bond interfaces supporting Jun 22, 2021
@ObjatieGroba ObjatieGroba requested a review from nMustaki June 22, 2021 11:38
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