-
Notifications
You must be signed in to change notification settings - Fork 874
generate RestoreObject #4208
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
base: peterrsongg/petesong/phase-3-pr5-rebased-2/4
Are you sure you want to change the base?
generate RestoreObject #4208
Conversation
stack-info: PR: #4208, branch: peterrsongg/petesong/phase-3-pr5-rebased-2/5
a1013dd to
ec4fd4a
Compare
108e71e to
925e059
Compare
| { "IntelligentTieringAndOperator", 2} | ||
| { "IntelligentTieringAndOperator", 2}, | ||
| //TODO: Add runbook entry as to why this excluded and how to do a customization for this. | ||
| { "GlacierJobParameters", 1 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GlacierParameters is a structure with only 1 member Tier. In our custom implementation we just moved the Tier member up two levels to the RestoreObjectRequest and also changed the type to be an enum. This means that if something is ever added to GlacierParameters we will miss it. This ensures that if any member is added to GlacierParameters we don't miss it and do the manual work necessary.
| } | ||
| } | ||
|
|
||
| private IList<Member> _requestPayloadFlattenMembers; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this wasn't being used so i just removed it.
READ THISThe 1st breaking change it is flagging isn't true. I added that customization to preserve existing behavior. Based on my comprehensive analysis of commit BREAKING CHANGES FOUND:1. CRITICAL: InputSerialization.cs - Default Value AdditionFile: Issue: The generated version introduces a default value that didn't exist in the custom version:
Evidence: This change is confirmed in "CompressionType":{"injectXmlPrivateMemberAssignment" :["private CompressionType _compressionType = CompressionType.None;"]}Impact: This is a breaking change because:
2. MINOR: S3Location.cs - Spacing Issue in IsSet MethodFile: Issue: Line 200 has a spacing issue in the IsSet method: return this._userMetadata!= null; // Missing space before !=Impact: While this should still compile and function correctly, it's a code quality issue that could indicate other potential problems. NON-BREAKING CHANGES VERIFIED:
SUMMARY:Files Analyzed: 34 out of 46 files changed in the commit
Critical Recommendation: The Remaining Files: The remaining 12 files (generator-related files, service enumeration changes, integration tests) are primarily infrastructure/tooling changes and do not introduce customer-facing breaking changes to the S3 API. |
…tesong/phase-3-pr5-rebased-2/5
stack-info: PR: #4208, branch: peterrsongg/petesong/phase-3-pr5-rebased-2/5
stack-info: PR: #4208, branch: peterrsongg/petesong/phase-3-pr5-rebased-2/5
| } | ||
|
|
||
| [TestMethod] | ||
| public void TestRestoreObjectWithTier() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to add more tests, specifically around OutputLocation and the ACL's but those fields are only valid for SELECT operations on restores. I couldn't run them locally but i verified that the xml we send is the same as the CLI for complex s3 select restores.
Description
Generates
RestoreObjectAssembly Comparison Output (new properties and types added, which isn't a breaking change)
The fuzz test caught one issue where I made a mistake in the TierCustomMarshall, but that has been fixed.
Motivation and Context
Testing
DRY RUN: DRY_RUN-e581806f-264b-46d0-a331-1318e27c2273
Screenshots (if appropriate)
Types of changes
Checklist
License