Skip to content

Conversation

@maeldonn
Copy link

Issue: CLDSRV-817

@maeldonn maeldonn requested review from a team, DarkIsDude, Copilot and delthas January 26, 2026 09:02
@bert-e
Copy link
Contributor

bert-e commented Jan 26, 2026

Hello maeldonn,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options
name description privileged authored
/after_pull_request Wait for the given pull request id to be merged before continuing with the current one.
/bypass_author_approval Bypass the pull request author's approval
/bypass_build_status Bypass the build and test status
/bypass_commit_size Bypass the check on the size of the changeset TBA
/bypass_incompatible_branch Bypass the check on the source branch prefix
/bypass_jira_check Bypass the Jira issue check
/bypass_peer_approval Bypass the pull request peers' approval
/bypass_leader_approval Bypass the pull request leaders' approval
/approve Instruct Bert-E that the author has approved the pull request. ✍️
/create_pull_requests Allow the creation of integration pull requests.
/create_integration_branches Allow the creation of integration branches.
/no_octopus Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
/unanimity Change review acceptance criteria from one reviewer at least to all reviewers
/wait Instruct Bert-E not to run until further notice.
Available commands
name description privileged
/help Print Bert-E's manual in the pull request.
/status Print Bert-E's current status in the pull request TBA
/clear Remove all comments from Bert-E from the history TBA
/retry Re-start a fresh build TBA
/build Re-start a fresh build TBA
/force_reset Delete integration branches & pull requests, and restart merge process from the beginning.
/reset Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

@bert-e
Copy link
Contributor

bert-e commented Jan 26, 2026

Incorrect fix version

The Fix Version/s in issue CLDSRV-817 contains:

  • None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

  • 9.2.22

  • 9.3.0

Please check the Fix Version/s of CLDSRV-817, or the target
branch of this pull request.

@codecov
Copy link

codecov bot commented Jan 26, 2026

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
8142 1 8141 0
View the full list of 1 ❄️ flaky test(s)
"after each" hook for "should create a bunch of objects and their versions"::put and head object with versioning With v4 signature "after each" hook for "should create a bunch of objects and their versions"

Flake rate in main: 100.00% (Passed 0 times, Failed 5 times)

Stack Traces | 0.857s run time
The bucket you tried to delete is not empty.

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@maeldonn maeldonn force-pushed the feature/CLDSRV-817/get-object-attributes branch from a7ad5b2 to 023e610 Compare January 26, 2026 09:08
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request adds support for the AWS S3 GetObjectAttributes API, which allows clients to retrieve object metadata without downloading the object itself. The implementation includes comprehensive unit and functional tests, a new API endpoint, header parsing utilities, and updates the Arsenal dependency to include the required support for this feature.

Changes:

  • Implements the GetObjectAttributes API endpoint with support for retrieving ETag, StorageClass, ObjectSize, ObjectParts, and Checksum attributes
  • Adds header parsing utility to validate and parse the x-amz-object-attributes header
  • Includes comprehensive unit and functional tests covering success cases, error conditions, and versioning scenarios

Reviewed changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
package.json Updates Arsenal dependency to feature branch and increments version to 9.2.22
yarn.lock Updates Arsenal lockfile entry to match feature branch reference
lib/api/objectGetAttributes.js New API implementation for GetObjectAttributes with versioning and delete marker support
lib/api/apiUtils/object/parseAttributesHeader.js New utility to parse and validate x-amz-object-attributes header
lib/api/api.js Registers the new objectGetAttributes API method
constants.js Adds allowedObjectAttributes Set defining valid attribute names
tests/unit/api/objectGetAttributes.js Unit tests covering API functionality, error cases, and versioning
tests/unit/api/apiUtils/object/parseAttributesHeader.js Unit tests for header parsing utility
tests/functional/aws-node-sdk/test/object/objectGetAttributes.js Functional tests using AWS SDK

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

"@azure/storage-blob": "^12.28.0",
"@hapi/joi": "^17.1.1",
"arsenal": "git+https://github.com/scality/Arsenal#8.2.43",
"arsenal": "git+https://github.com/scality/Arsenal#feature/ARSN-549/get-object-attributes",
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

Arsenal dependency points to a feature branch instead of a released version. The package.json and yarn.lock reference 'git+https://github.com/scality/Arsenal#feature/ARSN-549/get-object-attributes' rather than a stable release tag. Before merging this PR, the Arsenal dependency should be updated to point to a proper release version to avoid dependency management issues and ensure stability in production environments.

Suggested change
"arsenal": "git+https://github.com/scality/Arsenal#feature/ARSN-549/get-object-attributes",
"arsenal": "git+https://github.com/scality/Arsenal#8.2.0",

Copilot uses AI. Check for mistakes.
rateLimitDefaultBurstCapacity: 1,
rateLimitCleanupInterval: 10000, // 10 seconds
// Metadata allowed to be returned by getObjectAttributes API
allowedObjectAttributes: new Set([
Copy link
Author

Choose a reason for hiding this comment

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

Should we handle our custom metadata ?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, with the same additional permission @DarkIsDude added for ListObjectv2
→ should be done in a separate PR I guess

Copy link
Contributor

Choose a reason for hiding this comment

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

@maeldonn we need to handle indeed user metadata (and RestoreStatus?). Does it make sense for you @maeldonn ?

Copy link
Author

Choose a reason for hiding this comment

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

As we said, we will not process RestoreStatus but we will add user metadata in another ticket.

@delthas delthas removed their request for review January 26, 2026 11:10
@maeldonn maeldonn marked this pull request as draft January 26, 2026 13:58
@maeldonn maeldonn removed the request for review from DarkIsDude January 26, 2026 16:11
const { errorInstances } = require('arsenal');
const { allowedObjectAttributes } = require('../../../../constants');

function parseAttributesHeaders(headers) {
Copy link
Contributor

Choose a reason for hiding this comment

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

there is already a very similar function in @DarkIsDude PR, which efficiently handles validation & parsing: should we not "merge" both code (i.e. typically make your code extend his) ?

Copy link
Author

Choose a reason for hiding this comment

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

Will be updated in another PR (process user metadata in GetObjectAttributes).

Copy link
Contributor

Choose a reason for hiding this comment

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

why? it means we are duplicating the code now instead of just rebasing this PR on top of the other and handling it just once...

@maeldonn maeldonn force-pushed the feature/CLDSRV-817/get-object-attributes branch from d9b4211 to 3b7dddd Compare January 27, 2026 11:01
@maeldonn maeldonn marked this pull request as ready for review January 27, 2026 13:14
@maeldonn maeldonn force-pushed the feature/CLDSRV-817/get-object-attributes branch from 3b7dddd to 5a3c013 Compare January 27, 2026 13:29
@maeldonn maeldonn force-pushed the feature/CLDSRV-817/get-object-attributes branch from 5a3c013 to 23f1c1c Compare January 28, 2026 10:16
@bert-e
Copy link
Contributor

bert-e commented Jan 28, 2026

Incorrect fix version

The Fix Version/s in issue CLDSRV-817 contains:

  • None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

  • 9.2.23

  • 9.3.0

Please check the Fix Version/s of CLDSRV-817, or the target
branch of this pull request.

rateLimitDefaultBurstCapacity: 1,
rateLimitCleanupInterval: 10000, // 10 seconds
// Metadata allowed to be returned by getObjectAttributes API
allowedObjectAttributes: new Set([
Copy link
Contributor

Choose a reason for hiding this comment

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

@maeldonn we need to handle indeed user metadata (and RestoreStatus?). Does it make sense for you @maeldonn ?

@maeldonn maeldonn requested a review from DarkIsDude January 28, 2026 17:50
@bert-e
Copy link
Contributor

bert-e commented Jan 29, 2026

Incorrect fix version

The Fix Version/s in issue CLDSRV-817 contains:

  • None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

  • 9.2.24

  • 9.3.0

Please check the Fix Version/s of CLDSRV-817, or the target
branch of this pull request.

@bert-e
Copy link
Contributor

bert-e commented Jan 30, 2026

Incorrect fix version

The Fix Version/s in issue CLDSRV-817 contains:

  • None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

  • 9.2.25

  • 9.3.0

Please check the Fix Version/s of CLDSRV-817, or the target
branch of this pull request.

@maeldonn maeldonn force-pushed the feature/CLDSRV-817/get-object-attributes branch from c45c894 to 838227a Compare January 30, 2026 15:04
@maeldonn maeldonn force-pushed the feature/CLDSRV-817/get-object-attributes branch from 838227a to 15d2f81 Compare January 30, 2026 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants