longhorn-manager icon indicating copy to clipboard operation
longhorn-manager copied to clipboard

refactor(setting): add value range in setting definition and use it in validation

Open ChanYiLin opened this issue 1 year ago • 5 comments

ref: https://github.com/longhorn/longhorn/issues/7441

Some ideas:

  1. use map to present max and min value instead of use int directly.
    • Because we still want to omitempty and distinguish 0 or nil. We then need to use *int in the structure so it can show 0 correctly and omitempy when nil.
    • We then need to declare a value first and assign it to structure as pointer for every definition which might make code not clean.
      val := 0
      difinition := SettingDefinition {
          Minimum: &val
      }
      
    • Use map, we can avoid the above problem.
  2. For some multi-choice settings, we used another util function to validate it which is not correct
    • SettingNameOfflineReplicaRebuilding, SettingNameSnapshotDataIntegrity, SettingNameBackupCompressionMethod, SettingNameReplicaAutoBalance cc @derekbit @c3y1huang
    • In the setting here, they should be global setting, so they are not allowed to have setting like Ignored.
    • Ignored should be used in volume level setting for it to follow global setting.
    • Thus, we shouldn't use util validation function like ValidateReplicaAutoBalance() to validate it because it allows Ignored setting.
    • I moved these settings to multi-choice category, so they follow the choices inside the definition.Choice.
  3. I separated < 0 and < 1, they should be the same, but I think it make developers more clear about the setting and no need to look up the code.

ChanYiLin avatar Feb 23 '24 09:02 ChanYiLin

E2E test: https://ci.longhorn.io/job/private/job/longhorn-tests-regression/6460/console

ChanYiLin avatar Feb 26 '24 08:02 ChanYiLin

-vs test_settings.py E2E test PASSED: https://ci.longhorn.io/job/private/job/longhorn-tests-regression/6521/

ChanYiLin avatar Feb 27 '24 07:02 ChanYiLin

-m coretest E2E test PASSED: https://ci.longhorn.io/job/private/job/longhorn-tests-regression/6528/

ChanYiLin avatar Feb 27 '24 08:02 ChanYiLin

This pull request is now in conflict. Could you fix it @ChanYiLin? 🙏

mergify[bot] avatar Mar 18 '24 08:03 mergify[bot]

This pull request is now in conflict. Could you fix it @ChanYiLin? 🙏

Conflicts resolved.

ChanYiLin avatar Mar 18 '24 08:03 ChanYiLin