10000 item: deduplicate value from resolve decorator by SchoolGuy · Pull Request #3252 · cobbler/cobbler · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

item: deduplicate value from resolve decorator #3252

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
Merged
merged 3 commits into from
Jan 16, 2024
Merged

item: deduplicate value from resolve decorator #3252

merged 3 commits into from
Jan 16, 2024

Conversation

SchoolGuy
Copy link
Member

Linked Items

Fixes #3211
Superseedes #3214

Description

CC @geliwei

Behaviour changes

Old: Setters didn't deduplicate the dicts and lists

New: Setters for dicts and lists in properties now deduplicate properly

Category

This is related to a:

  • Bugfix
  • Feature
  • Packaging
  • Docs
  • Code Quality
  • Refactoring
  • Miscellaneous

Tests

  • Unit-Tests were created
  • System-Tests were created
  • Code is already covered by Unit-Tests
  • Code is already covered by System-Tests
  • No tests required

@SchoolGuy SchoolGuy added this to the v3.4.0 milestone Sep 12, 2022
@github-actions github-actions bot added the tests label Sep 12, 2022
@codecov

This comment was marked as duplicate.

Sign in to view
@SchoolGuy
Copy link
Member Author

Okay sadly the approach chosen here doesn't work since we pass the underlying value and not the property itself. This needs to be reworked.

@SchoolGuy SchoolGuy force-pushed the fix/deduplicate-duplicated-dicts-and-lists branch from 239e32a to 42fc27d Compare September 29, 2022 13:24
@SchoolGuy SchoolGuy force-pushed the fix/deduplicate-duplicated-dicts-and-lists branch from 42fc27d to c5ee9da Compare October 18, 2022 18:39
@SchoolGuy SchoolGuy force-pushed the fix/deduplicate-duplicated-dicts-and-lists branch 3 times, most recently from 1ad2866 to dcb19e8 Compare November 20, 2022 16:56
@SchoolGuy SchoolGuy force-pushed the fix/deduplicate-duplicated-dicts-and-lists branch from dcb19e8 to d0ad891 Compare December 2, 2022 17:21
@SchoolGuy
Copy link
Member Author

This should look better once #3292 is in.

@SchoolGuy SchoolGuy force-pushed the fix/deduplicate-duplicated-dicts-and-lists branch from d0ad891 to 38fff03 Compare December 3, 2022 22:39
@SchoolGuy
Copy link
Member Author

We need to discuss the changes in behavior or find a better way to deduplicate lists. The filename bug is very weird and I just don't understand it yet.

@SchoolGuy SchoolGuy requested a review from a team December 8, 2022 21:08
@SchoolGuy SchoolGuy marked this pull request as ready for review December 8, 2022 21:08
@SchoolGuy SchoolGuy force-pushed the fix/deduplicate-duplicated-dicts-and-lists branch from 38fff03 to 11a5b6a Compare December 13, 2022 12:45
@SchoolGuy SchoolGuy force-pushed the fix/deduplicate-duplicated-dicts-and-lists branch from 11a5b6a to 4ad6110 Compare February 14, 2023 19:16
@SchoolGuy SchoolGuy marked this pull request as draft March 4, 2023 21:34
@SchoolGuy SchoolGuy force-pushed the fix/deduplicate-duplicated-dicts-and-lists branch 3 times, most recently from 89fc73b to c11e449 Compare December 3, 2023 19:15
@ycedres
Copy link
Contributor
ycedres commented Dec 5, 2023

@SchoolGuy
I've been debugging test_resolved_dict_deduplication and this is what I get:

> /code/cobbler/items/item.py(464)__resolve_get_parent_or_settings()
-> if self.parent is not None and hasattr(self.parent, property_name):
(Pdb) p self.parent is None
True
(Pdb) p self
<cobbler.items.profile.Profile object at 0x7f8364d66a10>
(Pdb) n

So an AttributeError is raised here.

I don't know why this happens but I hope at least helps.

@SchoolGuy
Copy link
Member Author
SchoolGuy commented Dec 14, 2023 •
edited
Loading

Well this is interesting:

> /code/cobbler/tftpgen.py(703)get_profiles_menu()
-> boot_loaders = profile.boot_loaders
(Pdb) from cobbler.items.profile import Profile
(Pdb) getattr(Profile, "boot_loaders").fget(profile)
*** AttributeError: <class 'cobbler.items.profile.Profile'> "test_resolved_list_deduplication" inherits property "boot_loaders", but neither its parent norsettings have it
(Pdb) profile.distro.name
'test_resolved_list_deduplication'
(Pdb) profile.boot_loaders
['COLLECTION_TYPE', 'LOGICAL_INHERITANCE', 'TYPE_DEPENDENCIES', 'TYPE_NAME', '_Item__common_resolve', '_Item__find_compare', '_Item__is_dict_key', '_Item__resolve_get_parent_or_settings', '__annotations__', '__class__', '__delattr__', '__dict__', '__dir__', '__doc__', '__eq__', '__format__', '__ge__', '__getattr__', '__getattribute__', '__getstate__', '__gt__', '__hash__', '__init__', '__init_subclass__', '__le__', '__lt__', '__module__', '__ne__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__sizeof__', '__str__', '__subclasshook__', '__weakref__', '_autoinstall', '_autoinstall_meta', '_boot_files', '_boot_loaders', '_cache', '_children', '_clean_dict_cache', '_comment', '_ctime', '_deduplicate_dict', '_deduplicate_list', '_depth', '_dhcp_tag', '_display_name', '_distro', '_enable_ipxe', '_enable_menu', '_fetchable_files', '_filename', '_has_initialized', '_inmemory', '_is_subobject', '_kernel_options', '_kernel_options_post', '_last_cached_mtime', '_menu', '_mgmt_classes', '_mgmt_parameters', '_mtime', '_name', '_name_servers', '_name_servers_search', '_next_server_v4', '_next_server_v6', '_owners', '_parent', '_proxy', '_redhat_management_key', '_remove_depreacted_dict_keys', '_repos', '_resolve', '_resolve_dict', '_resolve_enum', '_resolve_list', '_server', '_template_files', '_uid', '_virt_auto_boot', '_virt_bridge', '_virt_cpus', '_virt_disk_driver', '_virt_file_size', '_virt_path', '_virt_ram', '_virt_type', 'api', 'arch', 'autoinstall', 'autoinstall_meta', 'boot_files', 'boot_loaders', 'cache', 'check_if_valid', 'children', 'clean_cache', 'comment', 'ctime', 'depth', 'descendants', 'deserialize', 'dhcp_tag', 'display_name', 'distro', 'dump_vars', 'enable_ipxe', 'enable_menu', 'fetchable_files', 'filename', 'find_match', 'find_match_single_key', 'from_dict', 'get_conceptual_parent', 'get_parent', 'grab_tree', 'inmemory', 'is_subobject', 'kernel_options', 'kernel_options_post', 'logger', 'logical_parent', 'make_clone', 'menu', 'mgmt_classes', 'mgmt_parameters', 'mtime', 'name', 'name_servers', 'name_servers_search', 'next_server_v4', 'next_server_v6', 'owners', 'parent', 'proxy', 'redhat_management_key', 'repos', 'serialize', 'server', 'sort_key', 'template_files', 'to_dict', 'tree_walk', 'uid', 'virt_auto_boot', 'virt_bridge', 'virt_cpus', 'virt_disk_driver', 'virt_file_size', 'virt_path', 'virt_ram', 'virt_type']
*** AttributeError: Attribute "boot_loaders" did not exist on object type Profile.
(Pdb) profile.boot_loaders = "test"
*** cobbler.cexceptions.CX: 'Error with profile "test_resolved_list_deduplication" - not all boot_loaders are supported (given:"[\'test\']"; supported: "[\'grub\', \'pxe\', \'ipxe\']")'

The property is attached to the class correctly, shows in the dir() output and the setter works as expected. The getter doesn't seem to work though. Other list properties are working as expected and only this one is refusing operation. Seems somewhere the getter is overwritten. The distro version of this property seems to work as expected.

Edit: Further debugging showed that the getter is attached just fine. This is just not transparent in the stacktrace.

@SchoolGuy SchoolGuy force-pushed the fix/deduplicate-duplicated-dicts-and-lists branch from c11e449 to 0bd2146 Compare December 14, 2023 16:34
@SchoolGuy
Copy link
Member Author
SchoolGuy commented Dec 14, 2023 •
edited
Loading

One step further:

> /code/cobbler/items/item.py(462)__resolve_get_parent_or_settings()
-> def __resolve_get_parent_or_settings(self, property_name: str, settings_name: str):
(Pdb) l
457                     f'{type(self)} "{self.name}" does not have property "{property_name}"'
458                 )
459  
460             return getattr(self, attribute), settings_name
461  
462  ->     def __resolve_get_parent_or_settings(self, property_name: str, settings_name: str):
463             settings = self.api.settings()
464             if self.parent is not None and hasattr(self.parent, property_name):
465                 return getattr(self.parent, property_name)
466             if hasattr(settings, settings_name):
467                 return getattr(settings, settings_name)
(Pdb) property_name
'boot_loaders'
(Pdb) settings_name
'boot_loaders'
(Pdb) self.parent is not None
False
(Pdb) hasattr(self.parent, property_name)
False
(Pdb) self.parent
(Pdb) self
<cobbler.items.profile.Profile object at 0x7f05cd7e7fd0>
(Pdb) self.distro
<cobbler.items.distro.Distro object at 0x7f05cd7e5cd0>

parent doesn't correctly redirect to distro it seems. As such the AttributeError.

Edit: Fixing that error fixes the tests almost. To get them green the assertions needed to contain the default settings values.

@SchoolGuy
Copy link
Member Author

Ah. It seems we have the same bug for System now. However, that is something for tomorrow.

@SchoolGuy SchoolGuy force-pushed the fix/deduplicate-duplicated-dicts-and-lists branch from d0a016b to 90f8657 Compare December 15, 2023 15:36
@SchoolGuy
Copy link
Member Author

hasattr accesses the getter of the property which in our test scenario leads to an exception. This means that hasattr returns False which in turn leads to the required test change for test_create_profile_negative.

@codacy-production Codacy Production
Copy link
codacy-production bot commented Dec 15, 2023 •
edited
Loading

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
✅ +0.06% ✅ 96.88%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (e626a52) 15963 10619 66.52%
Head commit (699892a) 15985 (+22) 10643 (+24) 66.58% (+0.06%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#3252) 64 62 96.88%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation

@SchoolGuy SchoolGuy force-pushed the fix/deduplicate-duplicated-dicts-and-lists branch from 90f8657 to de3ca4b Compare December 15, 2023 16:28
@SchoolGuy SchoolGuy force-pushed the fix/deduplicate-duplicated-dicts-and-lists branch from de3ca4b to 4291bd0 Compare December 15, 2023 17:28
@SchoolGuy SchoolGuy marked this pull request as ready for review December 15, 2023 17:28
@SchoolGuy SchoolGuy force-pushed the fix/deduplicate-duplicated-dicts-and-lists branch from 4291bd0 to 468ee57 Compare December 18, 2023 10:00
@SchoolGuy SchoolGuy mentioned this pull request Dec 18, 2023
Draft
12 tasks
@SchoolGuy SchoolGuy force-pushed the fix/deduplicate-duplicated-dicts-and-lists branch from 468ee57 to b998094 Compare December 19, 2023 14:10
agraul
agraul reviewed Dec 19, 2023
View reviewed changes
Copy link
Contributor
@agraul agraul left a comment

Choose a reason for hiding this comment

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

I'd like to experiment a bit with the problem as well, I feel like there is a simpler solution that does not require all the workarounds for finding a "parent".

@SchoolGuy
Copy link
Member Author

@agraul Feel free to experiment. I was hoping to get this PR in before the Christmas holiday to work on #3440 between the years but if doesn't work then that is completely fine to me as well.

@SchoolGuy SchoolGuy force-pushed the fix/deduplicate-duplicated-dicts-and-lists branch from b998094 to e6490b8 Compare December 19, 2023 14:50
@agraul
Copy link
Contributor
agraul commented Dec 19, 2023

@agraul Feel free to experiment. I was hoping to get this PR in before the Christmas holiday to work on #3440 between the years but if doesn't work then that is completely fine to me as well.

I think #3440 will cause changes to this one, I think you could already start before this gets in. But I try to have something tomorrow, at least to understand the constraints better.

@SchoolGuy
Copy link
Member Author

@agraul ah you think we should reverse the order of merging? I was thinking of getting this PR in first to do the refactoring without the bug being present. Don't rush your work, good things take time. I wanted to present my desire not set a fixed timeline. :)

6D40
@SchoolGuy SchoolGuy force-pushed the fix/deduplicate-duplicated-dicts-and-lists branch from e70c594 to 62d8821 Compare December 21, 2023 09:27
@SchoolGuy SchoolGuy requested a review from agraul December 21, 2023 09:57
SchoolGuy and others added 3 commits January 2, 2024 22:46
@SchoolGuy
Docker: Add pytest upgrade
9f8ea01
@SchoolGuy
item: deduplicate value from resolve decorator
0f9f8c8
@agraul @SchoolGuy
Use get_conceptual_parent() for resolving attrs
699892a 
`get_conceptual_parent()` contains the logic required to traverse the
inheritance chain. For resolving an inherited attribute, this method can
be re-used.

Additionally, redundant `hasattr()` checks are removed. These were only
used to raise an exception, that will now be raised in the succeeding
lines by calling `getattr()` without a default argument.
@SchoolGuy SchoolGuy force-pushed the fix/deduplicate-duplicated-dicts-and-lists branch from 62d8821 to 699892a Compare January 2, 2024 21:46
@ycedres
Copy link
Contributor
ycedres commented Jan 11, 2024

The way it is done now seems pretty reasonable FMPOV. @SchoolGuy what do you mean by reversing the order of merging?

@SchoolGuy
Copy link
Member Author

@ycedres There are currently two possibilities for the merge order on the table:

  1. Merge this PR and then Items: Refactor base classes #3440
  2. Merge Items: Refactor base classes #3440 and then this PR

I am in favour of the first option. Alex has proposed option two.

ycedres
ycedres approved these changes Jan 16, 2024
View reviewed changes
Copy link
Contributor
@ycedres ycedres left a comment

Choose a reason for hiding this comment

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

LGTM

@SchoolGuy SchoolGuy merged commit 54cdbd8 into main Jan 16, 2024
@SchoolGuy SchoolGuy deleted the fix/deduplicate-duplicated-dicts-and-lists branch January 16, 2024 15:36
@SchoolGuy SchoolGuy mentioned this pull request Jul 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ycedres ycedres ycedres approved these changes

@agraul agraul Awaiting requested review from agraul

Assignees
No one assigned
Labels
tests
Projects
Cobbler Server
Status: Done
Milestone
v3.4.0
Development

Successfully merging this pull request may close these issues.

Property Setters don't deduplicate in dict or list case
3 participants
@SchoolGuy @ycedres @agraul
0