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

NC | CLI | Fix for unsettable flags issues #8774

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

romayalon
Copy link
Contributor

@romayalon romayalon commented Feb 6, 2025

Explain the changes

This PR fixes 3 issues with CLI unsettable flags and a tiny test rename refactoring -

  1. We checked if a flag can be unsettable only if the expected type of the flag value and the actual type of value didn't match, but this is not the case when the flag type and value's type match - we should always check it.
  2. The list LIST_UNSETABLE_OPTIONS was missing some flags that were supposed to be included in it and be checked, their check passed because their type was string (See bullet 1).
    The missing options were new_buckets_path (was missing also in the help) and supplemental_groups.
  3. The list LIST_UNSETABLE_OPTIONS consisted s3_policy flag which does not exist, changed it to bucket_policy.
  4. Renamed test_nc_nsfs_account_cli.test.js -> test_nc_account_cli.test.js and it's references.

** Found this bug during the reviewing of - #8722, hopefully it can help @achouhan09 with the comment #8722 (comment)

Issues: Fixed #xxx / Gap #xxx

Testing Instructions:

  • Doc added/updated
  • Tests added

@romayalon romayalon force-pushed the romy-cli-fix-unset-issue branch from ad60d08 to aab5631 Compare February 6, 2025 15:14
@pull-request-size pull-request-size bot added size/M and removed size/S labels Feb 6, 2025
@romayalon romayalon force-pushed the romy-cli-fix-unset-issue branch from 8478075 to ffeee03 Compare February 6, 2025 16:23
@romayalon romayalon force-pushed the romy-cli-fix-unset-issue branch from d38e302 to f934496 Compare February 9, 2025 15:48
src/manage_nsfs/manage_nsfs_constants.js Outdated Show resolved Hide resolved
src/manage_nsfs/manage_nsfs_validations.js Outdated Show resolved Hide resolved
src/test/unit_tests/jest_tests/test_nc_account_cli.test.js Outdated Show resolved Hide resolved
src/test/unit_tests/test_nc_cli.js Outdated Show resolved Hide resolved
src/test/unit_tests/test_nc_cli.js Show resolved Hide resolved
src/test/unit_tests/test_nc_cli.js Outdated Show resolved Hide resolved
src/manage_nsfs/manage_nsfs_help_utils.js Outdated Show resolved Hide resolved
src/test/unit_tests/sudo_index.js Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/L and removed size/M labels Feb 10, 2025
@romayalon romayalon force-pushed the romy-cli-fix-unset-issue branch 2 times, most recently from e74f1e9 to 05a8ed9 Compare February 10, 2025 13:33
@romayalon romayalon requested a review from shirady February 10, 2025 14:34
Copy link
Contributor

@shirady shirady left a comment

Choose a reason for hiding this comment

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

LGTM

added a few minor comments

src/manage_nsfs/manage_nsfs_constants.js Outdated Show resolved Hide resolved
src/manage_nsfs/manage_nsfs_help_utils.js Outdated Show resolved Hide resolved
src/manage_nsfs/manage_nsfs_validations.js Outdated Show resolved Hide resolved
src/test/unit_tests/jest_tests/test_nc_account_cli.test.js Outdated Show resolved Hide resolved
src/test/unit_tests/test_nc_cli.js Show resolved Hide resolved
@romayalon romayalon force-pushed the romy-cli-fix-unset-issue branch from 3e19320 to 7dbd3dd Compare February 10, 2025 15:34
@romayalon romayalon force-pushed the romy-cli-fix-unset-issue branch from 7dbd3dd to 9f981fa Compare February 10, 2025 15:59
@romayalon romayalon merged commit 937075f into noobaa:master Feb 10, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants