Fix NOTEs on BucketReplicationConfigRules #5112
Conversation
Does the PR have any schema changes?Looking good! No breaking changes found. Maintainer note: consult the runbook for dealing with any breaking changes. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5112 +/- ##
=======================================
Coverage 24.46% 24.47%
=======================================
Files 361 361
Lines 144037 144065 +28
=======================================
+ Hits 35243 35260 +17
- Misses 108696 108707 +11
Partials 98 98 ☔ View full report in Codecov by Sentry. |
| @@ -351282,7 +351282,7 @@ | |||
| "items": { | |||
| "$ref": "#/types/aws:s3/BucketReplicationConfigRule:BucketReplicationConfigRule" | |||
| }, | |||
| "description": "List of configuration blocks describing the rules managing the replication. See below.\n" | |||
| "description": "List of configuration blocks describing the rules managing the replication. See below.\n\u003e **NOTE:** Replication to multiple destination buckets requires that `priority` is specified in the `rule` object. If the corresponding rule requires no filter, an empty configuration block `filter {}` must be specified.\n\n\u003e **NOTE:** Amazon S3's latest version of the replication configuration is V2, which includes the `filter` attribute for replication rules.\n\n\u003e **NOTE:** The `existing_object_replication` parameter is not supported by Amazon S3 at this time and should not be included in your `rule` configurations. Specifying this parameter will result in `MalformedXML` errors.\nTo replicate existing objects, please refer to the [Replicating existing objects with S3 Batch Replication](https://docs.aws.amazon.com/AmazonS3/latest/userguide/s3-batch-replication-batch.html) documentation in the Amazon S3 User Guide.\n" | |||
There was a problem hiding this comment.
We should probably use TypeScript notation here, not HCL.
| "description": "List of configuration blocks describing the rules managing the replication. See below.\n\u003e **NOTE:** Replication to multiple destination buckets requires that `priority` is specified in the `rule` object. If the corresponding rule requires no filter, an empty configuration block `filter {}` must be specified.\n\n\u003e **NOTE:** Amazon S3's latest version of the replication configuration is V2, which includes the `filter` attribute for replication rules.\n\n\u003e **NOTE:** The `existing_object_replication` parameter is not supported by Amazon S3 at this time and should not be included in your `rule` configurations. Specifying this parameter will result in `MalformedXML` errors.\nTo replicate existing objects, please refer to the [Replicating existing objects with S3 Batch Replication](https://docs.aws.amazon.com/AmazonS3/latest/userguide/s3-batch-replication-batch.html) documentation in the Amazon S3 User Guide.\n" | |
| "description": "List of configuration blocks describing the rules managing the replication. See below.\n\u003e **NOTE:** Replication to multiple destination buckets requires that `priority` is specified in the `rule` object. If the corresponding rule requires no filter, an empty configuration block `filter: {}` must be specified.\n\n\u003e **NOTE:** Amazon S3's latest version of the replication configuration is V2, which includes the `filter` attribute for replication rules.\n\n\u003e **NOTE:** The `existingObjectReplication` parameter is not supported by Amazon S3 at this time and should not be included in your `rule` configurations. Specifying this parameter will result in `MalformedXML` errors.\nTo replicate existing objects, please refer to the [Replicating existing objects with S3 Batch Replication](https://docs.aws.amazon.com/AmazonS3/latest/userguide/s3-batch-replication-batch.html) documentation in the Amazon S3 User Guide.\n" |
There was a problem hiding this comment.
nice catch! ty!
provider/doc_edits.go
Outdated
| @@ -134,6 +135,24 @@ var fixUpEcsServiceNameForceNewDeployment = targetedSimpleReplace( | |||
| "(e.g., `myimage:latest`), roll Fargate tasks onto a newer platform version, or immediately deploy "+ | |||
| "`ordered_placement_strategy` and `placement_constraints` updates.\n"+ | |||
| "When using the forceNewDeployment property you also need to configure the triggers property.\n") | |||
| var fixUpBucketReplicationConfig = targetedSimpleReplace( | |||
| "s3_bucket_replication_configuration.html.markdown", | |||
There was a problem hiding this comment.
Could we put a check in (a test) that ensures this content is matching upstream, if not take from upstream content directly? This way if upstream edits it away we'll remember to update the copy.
There was a problem hiding this comment.
We have hard failure patterns during make schema; I'm happy to add those. we should add them for the other fixes as well. It may be noisy during upgrades; that's the tradeoff. I think you're right to request this though.
There was a problem hiding this comment.
Not sure what the "hard failure patterns" are but appreciated - the change looks like we add content we own where in fact this is a workaround to propagate upstream content, this postpones the issue of possible discrepancies to the future, and I must assume this page is high visibility since we are attempting to patch it. Thanks!
There was a problem hiding this comment.
I've added a new function that follows the suggested pattern.
I'm going to follow up with a PR that addresses this for other large replaces.
| return tfbridge.DocsEdit{ | ||
| Path: filePath, | ||
| Edit: func(_ string, content []byte) ([]byte, error) { | ||
| if bytes.Contains(content, fromBytes) { |
There was a problem hiding this comment.
Unfortunately this does not ensure that toBytes does not digress, and yet we copied it from the source material. IN this case the toBytes is the bit that contains the notes we're copying.
There was a problem hiding this comment.
You're totally right. The new check is more specific to the situation and checks that both the added text and the fromBytes have not changed, else it errors.
…elsewhere have not changed, as well as ensure we can properly find the place to add to.
guineveresaenger
force-pushed
the
guin/bucketreplicationconfig
branch
from
7bd3805 to
b5f7f48
Compare
While this fix is not the complete underlying docsgen fix that would fix related issues, it will resolve the specific instance of difficulties experienced due to missing NOTEs on Rule.:wq
Fixes #4921.