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 | Config Dir Restructure #8279

Merged

Conversation

romayalon
Copy link
Contributor

@romayalon romayalon commented Aug 12, 2024

Explain the changes

This PR is based on @alphaprinz #8167 PR.

  1. Added to the new structure the following paths -
identities/
     {account_id}/account.json
     
accounts_by_name/
     {account_name}.symlink -> ../identities/{account_id}/account.json
  1. Re-implemented create/update/delete account in configFS to adhere the new config dir structure.
  2. Backwards compatibility -
  • Added backward compatibility to the following accounts getter functions -
  • get_account_by_name()
  • is_account_exists({name: string})
  • list_accounts()
  1. Updated getter callers to these functions to have { fallback:true } options.
  2. Converted tests to use config_fs.
  3. Added backward compatibility tests.

Issues: Fixed #xxx / Gap #xxx

  1. Add structure tests.
  2. Add list account backward compatibility tests after we agree on the expected result.
  3. Update Docs.
  4. Gap - When starting to use the new mapping

Testing Instructions:

  1. sudo npx jest --testRegex=jest_tests/test_config_fs_backward_compatibility.test.js
  • Doc added/updated
  • Tests added

@romayalon romayalon force-pushed the romy-config-dir-restructure-config-fs branch 5 times, most recently from 208e53c to d65e8f9 Compare August 13, 2024 13:48
@romayalon romayalon force-pushed the romy-config-dir-restructure-config-fs branch 2 times, most recently from e1bac3e to 516e256 Compare August 15, 2024 10:52
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.

Adding comments on the file config_fs

src/sdk/config_fs.js Outdated Show resolved Hide resolved
src/sdk/config_fs.js Outdated Show resolved Hide resolved
src/sdk/config_fs.js Show resolved Hide resolved
src/sdk/config_fs.js Outdated Show resolved Hide resolved
src/sdk/config_fs.js Outdated Show resolved Hide resolved
src/sdk/config_fs.js Outdated Show resolved Hide resolved
src/sdk/config_fs.js Outdated Show resolved Hide resolved
src/sdk/config_fs.js Outdated Show resolved Hide resolved
src/sdk/config_fs.js Outdated Show resolved Hide resolved
src/sdk/config_fs.js Show resolved Hide resolved
@romayalon romayalon mentioned this pull request Aug 18, 2024
2 tasks
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.

Adding additional comments.

src/cmd/manage_nsfs.js Outdated Show resolved Hide resolved
src/cmd/manage_nsfs.js Show resolved Hide resolved
src/cmd/manage_nsfs.js Outdated Show resolved Hide resolved
src/cmd/manage_nsfs.js Show resolved Hide resolved
src/manage_nsfs/manage_nsfs_validations.js Show resolved Hide resolved
src/test/system_tests/test_utils.js Show resolved Hide resolved
@shirady
Copy link
Contributor

shirady commented Aug 18, 2024

@romayalon, a question - after changing the file name to identity.json I thought we would also have identity_type in the JSONs, did I miss something?

@romayalon
Copy link
Contributor Author

@shirady I understood from @guymguym suggestion that we won't add it to account JSON, and when it's missing it's saying the identity is account, @guymguym did I understand correctly?

@romayalon romayalon force-pushed the romy-config-dir-restructure-config-fs branch from 8249fbf to 1f53a91 Compare August 18, 2024 16:45
src/sdk/config_fs.js Outdated Show resolved Hide resolved
src/sdk/config_fs.js Outdated Show resolved Hide resolved
src/sdk/config_fs.js Outdated Show resolved Hide resolved
src/sdk/config_fs.js Show resolved Hide resolved
src/sdk/config_fs.js Outdated Show resolved Hide resolved
src/sdk/config_fs.js Outdated Show resolved Hide resolved
src/sdk/config_fs.js Show resolved Hide resolved
src/sdk/config_fs.js Outdated Show resolved Hide resolved
src/test/system_tests/test_utils.js Outdated Show resolved Hide resolved
src/sdk/accountspace_fs.js Outdated Show resolved Hide resolved
src/sdk/config_fs.js Show resolved Hide resolved
src/sdk/config_fs.js Show resolved Hide resolved
@shirady
Copy link
Contributor

shirady commented Aug 19, 2024

@shirady I understood from @guymguym suggestion that we won't add it to account JSON, and when it's missing it's saying the identity is account, @guymguym did I understand correctly?

@romayalon I understand that we need to add it, but for the existing config files we will not have to add it because a missing property there means account.
Anyway, it is something that we will add when we will expand the identities (role, policy, etc.).

src/sdk/config_fs.js Outdated Show resolved Hide resolved
@romayalon romayalon force-pushed the romy-config-dir-restructure-config-fs branch from 97a94f7 to 34a348d Compare August 19, 2024 12:13
Comment on lines +342 to +344
if (err.code === 'ENOENT') {
dbg.warn(`delete_config_file: config file already deleted ${config_path}`);
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why doesn't it include the silent_if_missing condition? (like we have in folder_delete).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as we discussed, we always want on this case to not fail if the config file is missing

@romayalon romayalon force-pushed the romy-config-dir-restructure-config-fs branch from 34a348d to e2285af Compare August 19, 2024 13:08
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

@shirady
Copy link
Contributor

shirady commented Aug 19, 2024

@romayalon
2 things that we might want to discuss in the future:

  1. Do we want to add a property the accounts schema that will define the identity type for the account (link) - we would probably need it when we will plan to add additional property.
  2. In get_identity_by_id adding manually the error code ENOENT (link) - we need to examine it during the use if it creates somehow any confusion.

@romayalon romayalon merged commit 7015998 into noobaa:master Aug 19, 2024
10 checks passed
@romayalon romayalon mentioned this pull request Aug 19, 2024
2 tasks
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.

2 participants