Skip to content

NSFS | content dir versioning LIST and tagging operations #8814

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

Merged
merged 1 commit into from
May 6, 2025

Conversation

nadavMiz
Copy link
Contributor

@nadavMiz nadavMiz commented Feb 19, 2025

Explain the changes

  1. change list-object-versions to work with content directory objects
  2. change tagging functions to use _find_version_path in order to get the correct path
  3. remove configuration variable to enable content dir versioning, as it should be completed
  4. add documentation about content dir versioning
  5. add unit test

Issues: Fixed #xxx / Gap #xxx

Content-dir versioning issue: #8531
continuation of #8773

####GAPS

Testing Instructions:

  • Doc added/updated
  • Tests added

const fs_context = this.prepare_fs_context(object_sdk);
const file_path = await this._find_version_path(fs_context, params, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

have you checked that this change is compatible to S3? especially when versioning is suspended, delete tagging from the version id file is expected per the protocol?

Copy link
Contributor Author

@nadavMiz nadavMiz Mar 5, 2025

Choose a reason for hiding this comment

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

there shouldn't be a difference between enabled and suspended mode. but I will validate this. I will also check what happens if we are in disabled mode and have version as a parameter

Copy link
Contributor

Choose a reason for hiding this comment

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

hey did you check this?

Copy link
Contributor Author

@nadavMiz nadavMiz May 5, 2025

Choose a reason for hiding this comment

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

yes. it works in the same in suspended mode. its worth mentioning that for both enabled and suspended mode, it setobjecttagging failed without version-id with the following error:

An error occurred (MethodNotAllowed) when calling the PutObjectTagging operation: The specified method is not allowed against this resource.

I'll continue investigating this, but this seems more like a bucket policy quirk then anything else (there is nothing about it in the documentation for getObjectTagging)

anyway its out of scope for this PR

@romayalon
Copy link
Contributor

@nadavMiz please add a link in the description to all the content directory PRs the original issue, I think it's 8531

@nadavMiz nadavMiz force-pushed the directory-object-list-operation branch 2 times, most recently from 629a556 to b59fbb2 Compare April 2, 2025 09:50
@nadavMiz nadavMiz force-pushed the directory-object-list-operation branch from b59fbb2 to e20a6ac Compare May 4, 2025 14:49
@nadavMiz nadavMiz requested a review from romayalon May 4, 2025 14:50
@nadavMiz nadavMiz force-pushed the directory-object-list-operation branch 2 times, most recently from b738cb5 to b182aec Compare May 4, 2025 15:17
Copy link
Contributor

@romayalon romayalon left a comment

Choose a reason for hiding this comment

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

some minor comments, but looks good

if (this._is_hidden_version_path(obj_info.key)) {
obj_info.key = path.normalize(obj_info.key.replace(HIDDEN_VERSIONS_PATH + '/', ''));
obj_info.key = _get_filename(obj_info.key);
set_latest_delete_marker(obj_info);
Copy link
Contributor

Choose a reason for hiding this comment

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

should set_latest_delete_marker() function be a part of a function called format_key_name() ?

Copy link
Contributor Author

@nadavMiz nadavMiz May 5, 2025

Choose a reason for hiding this comment

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

right, it shouldn't. also putting it inside _is_hidden_version_path doesn't really add anything. I moved it outside

const fs_context = this.prepare_fs_context(object_sdk);
const file_path = await this._find_version_path(fs_context, params, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

hey did you check this?

@nadavMiz nadavMiz force-pushed the directory-object-list-operation branch 4 times, most recently from 1e975b3 to 67da1c7 Compare May 6, 2025 07:35
@nadavMiz nadavMiz force-pushed the directory-object-list-operation branch from 67da1c7 to 08fe0a3 Compare May 6, 2025 07:35
@nadavMiz nadavMiz merged commit 6787090 into noobaa:master May 6, 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.

2 participants