ARSN-528: add BucketLoggingStatus to BucketInfo model #2538

leif-scality · 2025-10-02T15:44:29Z

add BucketLoggingStatus to BucketInfo model

bert-e · 2025-10-02T15:44:32Z

Hello leif-scality,

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 · 2025-10-02T15:44:38Z

Incorrect fix version

The Fix Version/s in issue ARSN-528 contains:

None

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

8.2.33
8.3.0

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

codecov · 2025-10-02T15:56:48Z

Codecov Report

❌ Patch coverage is 55.35714% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.01%. Comparing base (fb44b3b) to head (4b4ea42).
⚠️ Report is 4 commits behind head on development/8.2.

Files with missing lines	Patch %	Lines
lib/models/BucketLoggingStatus.ts	62.85%	13 Missing ⚠️
lib/models/BucketInfo.ts	53.84%	6 Missing ⚠️
lib/s3routes/routes/routePUT.ts	20.00%	4 Missing ⚠️
lib/s3routes/routes/routeGET.ts	0.00%	2 Missing ⚠️

Additional details and impacted files

@@                 Coverage Diff                 @@
##           development/8.2    #2538      +/-   ##
===================================================
- Coverage            71.07%   71.01%   -0.06%     
===================================================
  Files                  220      221       +1     
  Lines                17729    17781      +52     
  Branches              3652     3688      +36     
===================================================
+ Hits                 12601    12628      +27     
- Misses                5124     5149      +25     
  Partials                 4        4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

jonathan-gramain

LGTM (I can review again in the final version)

jonathan-gramain · 2025-10-02T23:12:25Z

lib/models/BucketLoggingStatus.ts

+        } catch (err) {
+            return {
+                error: { arsenalError: errors.MalformedXML, details: err },
+                res: undefined,


I assume we probably don't need to add an undefined res property (if we do, then maybe we need to revisit how we interpret the results to avoid being too javascript-centric).

BourgoisMickael · 2025-10-03T10:49:21Z

lib/models/BucketInfo.ts

 import { AzureInfoMetadata } from './BucketAzureInfo';
+import BucketLoggingStatus from './BucketLoggingStatus';

 // WHEN UPDATING THIS NUMBER, UPDATE BucketInfoModelVersion.md CHANGELOG


You should update the doc https://github.com/scality/Arsenal/blob/development/8.2/documentation/BucketInfoModelVersion.md

and bump modelVersion = 18

Where should I do the bump?

1 or 2 line below there is the variable

BourgoisMickael · 2025-10-03T10:55:00Z

lib/models/BucketLoggingStatus.ts

+
+export type LoggingEnabled = {
+    TargetBucket: string;
+    TargetPrefix: string;


should we include optional TargetGrants and TargetObjectKeyFormat ? https://docs.aws.amazon.com/AmazonS3/latest/API/API_LoggingEnabled.html

nvm I see they are not implemented

Maybe add a comment in here to describe that those 2 fields are not implemented

BourgoisMickael · 2025-10-03T11:01:16Z

lib/models/BucketLoggingStatus.ts

+                return {
+                    error: {
+                        arsenalError: errors.NotImplemented,
+                        details: 'TargetGrants tag in LoggingEnabled is not implemented',


Suggested change

details: 'TargetGrants tag in LoggingEnabled is not implemented',

details: 'TargetGrants field in LoggingEnabled is not implemented',

is it aws that use tag instead of field in their error description ?

changed, XML uses tag and element but we can use field

BourgoisMickael · 2025-10-03T11:03:40Z

lib/models/BucketLoggingStatus.ts

+                        arsenalError: errors.NotImplemented,
+                        details: 'TargetObjectKeyFormat tag in LoggingEnabled is not implemented',


How is details going to be used ? Instead should we rather add a custom description directly on the error, to be returned to the client: errorInstances.NotImplemented.customDescription('...')

BourgoisMickael · 2025-10-03T11:05:30Z

lib/models/BucketLoggingStatus.ts

You can generate a unit test with AI to covers these

dvasilas · 2025-10-03T11:01:40Z

lib/models/BucketLoggingStatus.ts

+                };
+            }
+
+            if (parsed.BuckerLoggingStatus.LoggingEnabled.TargetGrants) {


Suggested change

if (parsed.BuckerLoggingStatus.LoggingEnabled.TargetGrants) {

if (parsed.BucketLoggingStatus.LoggingEnabled.TargetGrants) {

dvasilas · 2025-10-03T11:01:51Z

lib/models/BucketLoggingStatus.ts

+                };
+            }
+
+            if (parsed.BuckerLoggingStatus.LoggingEnabled.TargetObjectKeyFormat) {


Suggested change

if (parsed.BuckerLoggingStatus.LoggingEnabled.TargetObjectKeyFormat) {

if (parsed.BucketLoggingStatus.LoggingEnabled.TargetObjectKeyFormat) {

BourgoisMickael · 2025-10-03T11:12:40Z

lib/models/BucketLoggingStatus.ts

+                TargetPrefix: parsed.BucketLoggingStatus.LoggingEnabled.TargetPrefix[0],
+                TargetBucket: parsed.BucketLoggingStatus.LoggingEnabled.TargetBucket[0],


Aws performs no validation on those fields ?

Because bucket name is pretty restrictive format, and prefix should probably not go beyond a certains size

Maybe but the docs mentions no checks https://docs.aws.amazon.com/AmazonS3/latest/API/API_LoggingEnabled.html

…utes/routes

…uator/utils/actionsMaps.ts

leif-scality force-pushed the improvement/ARSN-528-add-bucketloggingstatus-model branch from e6293e8 to ef0c028 Compare October 2, 2025 15:48

dvasilas self-requested a review October 2, 2025 16:30

jonathan-gramain reviewed Oct 2, 2025

View reviewed changes

BourgoisMickael reviewed Oct 3, 2025

View reviewed changes

dvasilas reviewed Oct 3, 2025

View reviewed changes

BourgoisMickael reviewed Oct 3, 2025

View reviewed changes

leif-scality force-pushed the improvement/ARSN-528-add-bucketloggingstatus-model branch 2 times, most recently from fce43fb to 823a3ef Compare October 3, 2025 15:36

leif-scality added 3 commits October 3, 2025 17:43

ARSN-528: add BucketLoggingStatus to BucketInfo model

766e0b1

ARSN-528: add bucketPutLogging and bucketGetLogging route to lib/s3ro…

acb8616

…utes/routes

ARSN-528: add GetBucketLogging and PutBucketLogging to lib/policyEval…

edebec8

…uator/utils/actionsMaps.ts

leif-scality force-pushed the improvement/ARSN-528-add-bucketloggingstatus-model branch from 823a3ef to edebec8 Compare October 3, 2025 15:43

leif-scality added 2 commits October 3, 2025 18:39

tmp

e021e30

tmp2

4b4ea42

	details: 'TargetGrants tag in LoggingEnabled is not implemented',
	details: 'TargetGrants field in LoggingEnabled is not implemented',

		arsenalError: errors.NotImplemented,
		details: 'TargetObjectKeyFormat tag in LoggingEnabled is not implemented',

	if (parsed.BuckerLoggingStatus.LoggingEnabled.TargetGrants) {
	if (parsed.BucketLoggingStatus.LoggingEnabled.TargetGrants) {

		TargetPrefix: parsed.BucketLoggingStatus.LoggingEnabled.TargetPrefix[0],
		TargetBucket: parsed.BucketLoggingStatus.LoggingEnabled.TargetBucket[0],

ARSN-528: add BucketLoggingStatus to BucketInfo model #2538

Are you sure you want to change the base?

ARSN-528: add BucketLoggingStatus to BucketInfo model #2538

Uh oh!

Conversation

leif-scality commented Oct 2, 2025

Uh oh!

bert-e commented Oct 2, 2025

Hello leif-scality,

Uh oh!

bert-e commented Oct 2, 2025

Incorrect fix version

Uh oh!

codecov bot commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jonathan-gramain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Oct 2, 2025 •

edited

Loading