Skip to content

Conversation

@peterrsongg
Copy link
Contributor

@peterrsongg peterrsongg commented Dec 9, 2025

Description

Generates UpdateBucketMetadataTableConfiguration. There are breaking changes in this, but that is because the operation wasn't working before. This will be called out in the changelog as necessary to make the operation work. The engineer who made the customization mistakenly used the wrong shape as the modeled C# class.

Assembly Comparison Output (empty)

AssemblyComparer AWSSDK.S3.New.dll: Error Amazon.S3.Model.JournalTableConfigurationUpdates/MethodRemoved: Missing method Amazon.S3.JournalConfigurationState get_ConfigurationState() in Amazon.S3.Model.JournalTableConfigurationUpdates
AssemblyComparer AWSSDK.S3.New.dll: Error Amazon.S3.Model.JournalTableConfigurationUpdates/MethodRemoved: Missing method System.Void set_ConfigurationState(Amazon.S3.JournalConfigurationState) in Amazon.S3.Model.JournalTableConfigurationUpdates
AssemblyComparer AWSSDK.S3.New.dll: Error Amazon.S3.Model.JournalTableConfigurationUpdates/MethodRemoved: Missing method System.Void set_EncryptionConfiguration(Amazon.S3.Model.MetadataTableEncryptionConfiguration) in Amazon.S3.Model.JournalTableConfigurationUpdates
AssemblyComparer AWSSDK.S3.New.dll: Message Amazon.S3.Model.JournalTableConfigurationUpdates/MethodAdded: New method Amazon.S3.Model.RecordExpiration get_RecordExpiration() in Amazon.S3.Model.JournalTableConfigurationUpdates
AssemblyComparer AWSSDK.S3.New.dll: Message Amazon.S3.Model.JournalTableConfigurationUpdates/MethodAdded: New method System.Void set_RecordExpiration(Amazon.S3.Model.RecordExpiration) in Amazon.S3.Model.JournalTableConfigurationUpdates
AssemblyComparer AWSSDK.S3.New.dll: Error Amazon.S3.Model.JournalTableConfigurationUpdates/MethodRemoved: Missing method Amazon.S3.Model.MetadataTableEncryptionConfiguration get_EncryptionConfiguration() in Amazon.S3.Model.JournalTableConfigurationUpdates

Workload updates are available. Run `dotnet workload list` for more information.

Fuzz tests don't apply here since the request fails

Motivation and Context

Testing

I manually tested this operation currently and found it didn't work. A manual test of the newly generated operation succeeded.
DRY_RUN-e581806f-264b-46d0-a331-1318e27c2273

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed

License

  • I confirm that this pull request can be released under the Apache 2 license

stack-info: PR: #4206, branch: peterrsongg/petesong/phase-3-pr5-rebased-2/3
@peterrsongg peterrsongg force-pushed the peterrsongg/petesong/phase-3-pr5-rebased-2/3 branch from 32a5768 to 50bdfc2 Compare December 9, 2025 01:47
@peterrsongg peterrsongg force-pushed the peterrsongg/petesong/phase-3-pr5-rebased-2/2 branch from d1f15c3 to ca9c530 Compare December 9, 2025 01:47
peterrsongg added a commit that referenced this pull request Dec 9, 2025
stack-info: PR: #4206, branch: peterrsongg/petesong/phase-3-pr5-rebased-2/3
@peterrsongg peterrsongg marked this pull request as ready for review December 9, 2025 17:28
Copy link
Contributor

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 PR fixes the UpdateBucketMetadataJournalTableConfiguration operation by replacing incorrect handwritten code with properly generated code from the S3 service model. The original custom implementation used the wrong API shape, causing the operation to fail. The new generated code uses RecordExpiration instead of the incorrect ConfigurationState and EncryptionConfiguration properties.

Key Changes:

  • Replaced custom JournalTableConfigurationUpdates class with generated version that correctly uses RecordExpiration property
  • Generated proper request/response marshallers replacing handwritten implementations
  • Added operation to S3 allow list in ServiceModel.cs for code generation
  • Added generator customizations for ContentMD5 and ExpectedBucketOwner properties

Reviewed changes

Copilot reviewed 6 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
sdk/src/Services/S3/Generated/Model/UpdateBucketMetadataJournalTableConfigurationResponse.cs Updated response class with standard generated code formatting and headers
sdk/src/Services/S3/Generated/Model/UpdateBucketMetadataJournalTableConfigurationRequest.cs New generated request class with correct properties and comprehensive documentation
sdk/src/Services/S3/Generated/Model/JournalTableConfigurationUpdates.cs New generated class with correct RecordExpiration property instead of incorrect properties
sdk/src/Services/S3/Generated/Model/Internal/MarshallTransformations/UpdateBucketMetadataJournalTableConfigurationResponseUnmarshaller.cs New generated response unmarshaller with proper error handling
sdk/src/Services/S3/Generated/Model/Internal/MarshallTransformations/UpdateBucketMetadataJournalTableConfigurationRequestMarshaller.cs New generated request marshaller with correct XML serialization logic
sdk/src/Services/S3/Custom/Model/UpdateBucketMetadataJournalTableConfigurationRequest.cs Deleted incorrect custom implementation
sdk/src/Services/S3/Custom/Model/JournalTableConfigurationUpdates.cs Deleted incorrect custom implementation with wrong properties
sdk/src/Services/S3/Custom/Model/Internal/MarshallTransformations/UpdateBucketMetadataJournalTableConfigurationResponseUnmarshaller.cs Deleted custom unmarshaller implementation
sdk/src/Services/S3/Custom/Model/Internal/MarshallTransformations/UpdateBucketMetadataJournalTableConfigurationRequestMarshaller.cs Deleted custom marshaller implementation
generator/ServiceModels/s3/s3.customizations.json Added customizations for ContentMD5 and ExpectedBucketOwner IsSet methods
generator/ServiceClientGeneratorLib/ServiceModel.cs Added operation to S3 allow list for code generation

Comment on lines +579 to +580
new Operation(this, "ListObjects", DocumentRoot[OperationsKey]["ListObjects"]),
new Operation(this,"UpdateBucketMetadataJournalTableConfiguration", DocumentRoot[OperationsKey]["UpdateBucketMetadataJournalTableConfiguration"])
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

This PR is missing a required dev config file. According to CONTRIBUTING.md, all pull requests that modify code MUST include a DevConfig file in generator/.DevConfigs/ directory. This is essential for the release process and changelog generation.

For this change, you should create a dev config with a "major" type (breaking change) since the AssemblyComparer output shows removed methods (ConfigurationState, EncryptionConfiguration) and the operation signature has changed. Example structure:

{
  "services": [
    {
      "serviceName": "S3",
      "type": "major",
      "changeLogMessages": [
        "**Breaking Change** Fixed UpdateBucketMetadataJournalTableConfiguration operation to use correct API shape. Previous implementation used incorrect properties and did not work. Update code to use RecordExpiration property instead of ConfigurationState and EncryptionConfiguration."
      ],
      "backwardIncompatibilitiesToIgnore": [
        "Amazon.S3.Model.JournalTableConfigurationUpdates/MethodRemoved"
      ]
    }
  ]
}

See https://github.com/aws/aws-sdk-net/blob/main/CONTRIBUTING.md for details.

Copilot generated this review using guidance from repository custom instructions.
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.

1 participant