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

parse: Add the filepath to OVS ports netdefs #295

Merged
merged 3 commits into from
Jan 3, 2023

Conversation

daniloegea
Copy link
Collaborator

The filepath isn't being populated for OVS ports netdefs.

Add a unit test to catch this issue.

New unit test outcome without the fix:

======================================================================                                                                 
FAIL: test_filepath_for_ovs_ports (test_libnetplan.TestNetDefinition)                                                                                                    
----------------------------------------------------------------------                                                                 
Traceback (most recent call last):
  File "/workspace/netplan-filepath/tests/test_libnetplan.py", line 382, in test_filepath_for_ovs_ports
    self.assertEqual(os.path.join(self.confdir, "a.yaml"), netdef.filepath)
    AssertionError: '/tmp/tmpp3mx4zw6/etc/netplan/a.yaml' != None

Checklist

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad.

@slyon slyon self-requested a review November 14, 2022 15:00
Copy link
Collaborator

@slyon slyon left a comment

Choose a reason for hiding this comment

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

Thank you for spotting this issue and coming up with a fix right away, incl. a unit test!

I was wondering if we could actually move the assignment of component->filepath into netplan_netdef_new() instead, so we wouldn't have to assign it in different places.

But the problem is with one YAML file overriding another... Could you please extend the test case to e.g. have a.yaml defining the patch ports (openvswitch.ports) and having another (higher order) b.yaml file, re-defining the global OVS settings. What would be the value of filepath? I assume it would be a.yaml whereas it should be b.yaml.

Currently, the filepath is only set if the component does not yet exist, while we'd want it to be update if the component is being overriden by an higher order config file.

@daniloegea
Copy link
Collaborator Author

You are right! My patch wasn't updating the filepath in case of redefinition.

About setting the filepath in the netplan_netdef_new() function, we could do that. But the filepath update still must be done in the handle_*() functions as we are not creating new netdefs in case of redefinition.

@daniloegea daniloegea requested a review from slyon December 5, 2022 13:15
daniloegea and others added 3 commits January 3, 2023 12:46
The filepath isn't being populated for OVS ports netdefs.

Add a unit test to catch this issue.
When the object is redefined by a different yaml file, the filepath must
be updated to refer to that file.

Add a unit test to check for this invariant.
Copy link
Collaborator

@slyon slyon left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM.
I just rebased it on top of origin/main. Let's wait for the CI to pass, before merging it.

@slyon slyon merged commit c193697 into canonical:main Jan 3, 2023
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