8000 Convert Cobbler Item FIELDS array of arrays to Python properties by SchoolGuy · Pull Request #2433 · cobbler/cobbler · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Convert Cobbler Item FIELDS array of arrays to Python properties #2433

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 10 commits into from
Jun 10, 2021
Merged

Convert Cobbler Item FIELDS array of arrays to Python properties #2433

merged 10 commits into from
Jun 10, 2021

Conversation

SchoolGuy
Copy link
Member
@SchoolGuy SchoolGuy commented Oct 19, 2020 •
edited
Loading

Currently our testing is very very hard because we have circular dependencies and we did not organize our code very well. This means that changes are hard to introduce and we can't be certain if something works until the code is actually running. This makes working with the code very unfriendly has you need deep knowledge about Cobbler internals to make any change.

If we simplify our architecture and make our data storage objects not dependent on our main "data-collector-controller" we can:

  • test easier
  • introduce changes easier
  • switch to an orm at some point or make serializing objects way more easy then now

This means that this PR will be huge and messy. It will stay open for long as this means that I need to fix every usage in the Cobbler code and while doing that don't change external APIs to maintain backward compability but in my eyes it is worth the effort.

Below please see our current dependency net and the reason why I am so insane to even start this.

cobbler

Requirements to get this PR merged:

  • All touched code is tested and covered
  • Codacy does not complain about touched code, only existing one may have complaints.
  • @nodeg gives the green light
  • We test this against Uyuni Head
  • Breaking changes are documented
  • We will analyze if there are ORMs which could be used starting from 4.0 onwards with this approach and document the findings in a separate Issue.

@SchoolGuy SchoolGuy added Usability documentation Improvements or additions to documentation tech-debt Packaging Issues related to packaging labels Oct 19, 2020
@SchoolGuy SchoolGuy requested review from brejoc and watologo1 October 19, 2020 16:29
@SchoolGuy SchoolGuy self-assigned this Oct 19, 2020
@SchoolGuy
Copy link
Member Author

A hard requirement for myself in this PR is that the method signatures of remote.py and api.py do not change as these are our external points of entry for users and programs. This means also that the tests for these classes (meaning our integration tests) are not allowed to be adjusted and still need to pass.

@SchoolGuy
Copy link
Member Author

This PR is POST 3.2.0!

@SchoolGuy SchoolGuy marked this pull request as draft October 19, 2020 17:06
@SchoolGuy SchoolGuy added this to the v3.3.0 milestone Nov 18, 2020
@SchoolGuy SchoolGuy force-pushed the remove-circular-dependency-collection-manager branch from 1a782f2 to 8e904ac Compare March 5, 2021 07:56
@SchoolGuy SchoolGuy marked this pull request as ready for review March 5, 2021 08:04
@SchoolGuy
Copy link
Member Author

The current master branch structure is now

cobbler_deps_sync_systems

@SchoolGuy
Copy link
Member Author

This was generated with pydeps

@SchoolGuy SchoolGuy force-pushed the remove-circular-dependency-collection-manager branch from 8e904ac to 4d1874a Compare May 19, 2021 13:10
@SchoolGuy
Copy link
Member Author

Rebase onto master

@SchoolGuy SchoolGuy requested review from nodeg and removed request for brejoc May 19, 2021 13:11
@SchoolGuy
Copy link
Member Author
SchoolGuy commented May 19, 2021 •
edited
Loading

So the solution we want to go is the following:

  • Models will be converted from FIELDS to Python Properties
  • Shared validators will be put into the already existing validate.py
  • validate.py will be self contained in regard to dependencies possibly being added from Cobbler. External ones may be added.
  • All Items will move shared logic into validate.py.

This is not yet implemented so we need to try this approach out now.

EDIT: Above was changed to match this comment

@SchoolGuy SchoolGuy force-pushed the remove-circular-dependency-collection-manager branch 8 times, most recently from 6a3c333 to 5127a34 Compare May 25, 2021 12:37
@SchoolGuy
Copy link
Member Author

Resolved confilcts from master.

@SchoolGuy SchoolGuy force-pushed the remove-circular-dependency-collection-manager branch from 80f3dcf to 824e95a Compare June 9, 2021 11:40
@SchoolGuy SchoolGuy changed the title WIP: Convert Cobbler Item FIELDS array of arrays to Python properties Convert Cobbler Item FIELDS array of arrays to Python properties Jun 9, 2021
@SchoolGuy SchoolGuy force-pushed the remove-circular-dependency-collection-manager branch from 824e95a to f0ada5a Compare June 9, 2021 13:21
@SchoolGuy SchoolGuy force-pushed the remove-circular-dependency-collection-manager branch 2 times, most recently from 0a27d04 to b2ec04d Compare June 9, 2021 13:40
nodeg
nodeg requested changes Jun 10, 2021
View reviewed changes
Copy link
Member
@nodeg nodeg left a comment

Choose a reason for hiding this comment

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

This PR is bigger than I thought. I left some suggestions and improvements.

@SchoolGuy SchoolGuy force-pushed the remove-circular-dependency-collection-manager branch 2 times, most recently from f8b45b9 to 3d79744 Compare June 10, 2021 11:56
@SchoolGuy SchoolGuy requested a review from nodeg June 10, 2021 11:58
@SchoolGuy SchoolGuy force-pushed the remove-circular-dependency-collection-manager branch 3 times, most recently from 79df0a5 to a1375f3 Compare June 10, 2021 12:50
@SchoolGuy @nodeg
Fixup PR and addressed review concerns from @nodeg
22c56d9 
Co-authored-by: Dominik Gedon <dgedon@suse.de>
@SchoolGuy SchoolGuy force-pushed the remove-circular-dependency-collection-manager branch from a1375f3 to 22c56d9 Compare June 10, 2021 13:18
nodeg
nodeg requested changes Jun 10, 2021
View reviewed changes
Copy link
Member
@nodeg nodeg left a comment

Choose a reason for hiding this comment

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

Some more findings to check.

@SchoolGuy @nodeg
Address second review by @nodeg
db2e7bf 
Co-authored-by: Dominik Gedon <dgedon@suse.de>
nodeg
nodeg approved these changes Jun 10, 2021
View reviewed changes
Copy link
Member
@nodeg nodeg 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 70144e0 into cobbler:master Jun 10, 2021
@SchoolGuy SchoolGuy deleted the remove-circular-dependency-collection-manager branch July 6, 2021 07:47
@SchoolGuy SchoolGuy mentioned this pull request Jul 6, 2021
Merged
@btravouillon btravouillon mentioned this pull request Jun 4, 2022
Closed
@SchoolGuy SchoolGuy mentioned this pull request Oct 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nodeg nodeg nodeg approved these changes

@watologo1 watologo1 Awaiting requested review from watologo1

Assignees

@SchoolGuy SchoolGuy

Labels
documentation Improvements or additions to documentation Hacktoberfest-accepted tech-debt Usability
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@SchoolGuy @hbokh @nodeg
0