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

Fix issues on settings migrations and migrate collections to latest 3.3.0 schema. #3258

Merged
merged 7 commits into from
Oct 21, 2022

Conversation

meaksh
Copy link
Member

@meaksh meaksh commented Sep 23, 2022

Linked Items

Description

This PR fixes some issues around settings and migrations:

  • Enhance cobbler-settings command to allow passing settings to exclude from migration with --preserve-settings. This will fix Allow settings of custom plugins to be preserved with migrations #3261 and settings.d mix prevents upgrade from 3.1.2 to 3.3.0 #3262)
  • Execute migration of stored Cobbler collections as part of settings migrations (a backup of collections will be created).
  • Introduce a new settings named extra_settings_list to define list of custom settings the user wants to exclude from the migration/validation.
  • Migrate content of power_management_default_type setting from ipmitool to ipmilanplus
  • Remove manage_tftp settings after version 3.2.1.

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

Copy link
Member

@SchoolGuy SchoolGuy left a comment

Choose a reason for hiding this comment

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

Some questions, some frowning but overall looking already quite good.

cobbler/settings/migrations/V3_2_1.py Outdated Show resolved Hide resolved
cobbler/settings/migrations/V3_2_1.py Outdated Show resolved Hide resolved
cobbler/settings/migrations/V3_3_0.py Outdated Show resolved Hide resolved
cobbler/settings/migrations/V3_3_0.py Outdated Show resolved Hide resolved
@meaksh meaksh changed the title Fix issues on settings migrations and migrate collections to latest 3.3.0 schema. WIP: Fix issues on settings migrations and migrate collections to latest 3.3.0 schema. Sep 28, 2022
@SchoolGuy SchoolGuy marked this pull request as draft September 29, 2022 08:34
@SchoolGuy SchoolGuy linked an issue Sep 29, 2022 that may be closed by this pull request
@SchoolGuy SchoolGuy added this to the v3.4.0 milestone Sep 29, 2022
@meaksh meaksh force-pushed the main-fix-settings-migrations branch 2 times, most recently from ff7356c to a4518b1 Compare September 29, 2022 16:00
@SchoolGuy SchoolGuy linked an issue Sep 30, 2022 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Oct 3, 2022

Codecov Report

Base: 65.82% // Head: 65.72% // Decreases project coverage by -0.10% ⚠️

Coverage data is based on head (c3b20e9) compared to base (2a5c7ce).
Patch coverage: 57.51% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3258      +/-   ##
==========================================
- Coverage   65.82%   65.72%   -0.11%     
==========================================
  Files         112      113       +1     
  Lines       15006    15151     +145     
==========================================
+ Hits         9878     9958      +80     
- Misses       5128     5193      +65     
Impacted Files Coverage Δ
cobbler/settings/migrations/V3_1_2.py 82.35% <ø> (-11.77%) ⬇️
cobbler/settings/migrations/V3_2_0.py 85.00% <ø> (-10.00%) ⬇️
cobbler/settings/migrations/V3_3_1.py 95.00% <ø> (ø)
cobbler/settings/migrations/V3_3_3.py 93.33% <ø> (ø)
cobbler/settings/migrations/V3_4_0.py 87.50% <ø> (ø)
cobbler/settings/migrations/V3_3_0.py 48.54% <26.76%> (-48.34%) ⬇️
cobbler/settings/migrations/__init__.py 69.09% <68.29%> (-0.26%) ⬇️
cobbler/settings/__init__.py 92.93% <100.00%> (+4.82%) ⬆️
cobbler/settings/migrations/V3_2_1.py 84.84% <100.00%> (-4.44%) ⬇️
... and 17 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@meaksh meaksh changed the title WIP: Fix issues on settings migrations and migrate collections to latest 3.3.0 schema. Fix issues on settings migrations and migrate collections to latest 3.3.0 schema. Oct 3, 2022
@meaksh meaksh marked this pull request as ready for review October 3, 2022 15:46
Copy link
Member

@SchoolGuy SchoolGuy left a comment

Choose a reason for hiding this comment

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

The Git history is very messy. Please group the commits in a way that one can understand the changes by looking at the individual commits.

@meaksh meaksh force-pushed the main-fix-settings-migrations branch from 1bdf263 to 9a08da5 Compare October 5, 2022 08:40
@meaksh
Copy link
Member Author

meaksh commented Oct 5, 2022

@SchoolGuy I've rebased the commits and I hope commit history is better now.

@meaksh
Copy link
Member Author

meaksh commented Oct 5, 2022

Wait, I found some issue, something was last during the rebase. I'll update the branch again in a moment.

@meaksh meaksh force-pushed the main-fix-settings-migrations branch from 9a08da5 to dae47c2 Compare October 5, 2022 08:51
@meaksh
Copy link
Member Author

meaksh commented Oct 5, 2022

@SchoolGuy this should be fine now 👍

@SchoolGuy
Copy link
Member

@meaksh Codacy has some very valid concerns about your PR. Please have a look at them. For example the fact that you use lists as default arguments is problematic. (This was a bug in another part of Cobbler already.)

@meaksh meaksh force-pushed the main-fix-settings-migrations branch from 12b42a2 to c9f8ec6 Compare October 5, 2022 13:16
The migration of stored Cobbler collections will now happen
as part of the migration of 3.3.0 settings.

Additionally it fixes possible old ipmitool setting to new default.

A backup from old collections will be created at /var/lib/cobbler/
This commit introduces new logic that allows to preserve
and exclude custom extra settings from validation and migration.

A new parameter has been introduced to cobbler-settings, "--preserve-settings"
as well as a new settings named "extra_settings_list" to allow
defining your own custom settings to be excluded from validation.
Also around handling extra_settings_list settings

Additionally this fixes small issues found.
@meaksh meaksh force-pushed the main-fix-settings-migrations branch from c9f8ec6 to bcf24bc Compare October 6, 2022 14:56
Copy link
Member

@SchoolGuy SchoolGuy left a comment

Choose a reason for hiding this comment

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

Please investigate why Debian 10 and Rocky Linux 8 reliably fail with your PR. I previously overlooked that sadly. Please also mind the rest of the comments I now was able to spot. The Git history looks very nice now!

cobbler/settings/__init__.py Outdated Show resolved Hide resolved
cobbler/settings/migrations/V3_3_0.py Outdated Show resolved Hide resolved
cobbler/settings/migrations/__init__.py Outdated Show resolved Hide resolved
Additionally also uses proper Tuple for type annotation
@meaksh meaksh force-pushed the main-fix-settings-migrations branch from 00ba798 to 8fd572e Compare October 21, 2022 10:55
If "next_server" is a hostname and not an IPv4 nor IPv6 address
then we try to resolve this hostname to fill the corresponding
new "next_server_v4" and "next_server_v6" settings.

If resolution does not work for any of those settings, and error
is printed and migration of "next_server" is not performed.
@meaksh meaksh force-pushed the main-fix-settings-migrations branch from 8fd572e to c3b20e9 Compare October 21, 2022 11:50
Copy link
Member

@SchoolGuy SchoolGuy 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 5911207 into main Oct 21, 2022
@SchoolGuy SchoolGuy deleted the main-fix-settings-migrations branch October 21, 2022 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
2 participants