feat(opensearchservice): L2 properties for offPeakWindowOptions and softwareUpdateOptions by lpizzinidev · Pull Request #26403 · aws/aws-cdk

lpizzinidev · 2023-07-18T12:18:20Z

The OffPeakWindowOptions and SoftwareUpdateOptions are supported by OpenSearch Domain, but not by the CDK high-level construct.

This change adds the corresponding properties to the Domain construct:

const domain = new Domain(this, 'Domain', {
  version: EngineVersion.OPENSEARCH_1_3,
  offPeakWindowEnabled: true, // can be omitted if offPeakWindowStart is set
  offPeakWindowStart: {
    hours: 20,
    minutes: 0,
  },
  enableAutoSoftwareUpdate: true,
});

Closes #26388.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

…oftwareUpdateOptions properties

kaizencc

Looks pretty good @lpizzinidev! Just a couple comments about defaults

packages/aws-cdk-lib/aws-opensearchservice/lib/domain.ts

kaizencc · 2023-07-21T18:00:03Z

packages/aws-cdk-lib/aws-opensearchservice/lib/domain.ts

+   * Options for a domain's off-peak window, during which OpenSearch Service can perform mandatory
+   * configuration changes on the domain.
+   */
+  readonly offPeakWindowOptions?: OffPeakWindowOptions;


specifically the default for this prop may be confusing. looks like the default should be that it is configured by default and cannot be turned off, but still, what will the default window be?

Off-peak windows were introduced on February 16, 2023. All domains created before this date have the off-peak window disabled by default. You must manually enable and configure the off-peak window for these domains. All domains created after this date will have the off-peak window enabled by default. You can't disable the off-peak window for a domain after it's enabled.

https://docs.aws.amazon.com/opensearch-service/latest/developerguide/off-peak.html

I defaulted it to 00:00 UTC

If you don't specify a custom window start time, it defaults to 00:00 UTC.

https://docs.aws.amazon.com/opensearch-service/latest/developerguide/off-peak.html (in Enabling the off-peak window > CLI)

Pull request has been modified.

kaizencc

Thanks @lpizzinidev, a couple more comments

kaizencc · 2023-07-24T19:20:55Z

packages/aws-cdk-lib/aws-opensearchservice/lib/domain.ts

+/**
+ * Options for configuring service software updates for a domain.
+ */
+export interface SoftwareUpdateOptions {


i dont think we need an interface for this, lets default to a flatter API. Think we should just expose a boolean enableAutoSoftwareUpdate

kaizencc · 2023-07-24T19:27:35Z

packages/aws-cdk-lib/aws-opensearchservice/lib/domain.ts

+export interface OffPeakWindowOptions {
+  /**
+   * Specifies whether off-peak window settings are enabled for the domain.
+   *
+   * @default - true
+   */
+  readonly enabled?: boolean;
+
+  /**
+   * Off-peak window settings for the domain.
+   *
+   * @default - default window start time is 00:00 UTC
+   */
+  readonly offPeakWindow?: OffPeakWindow;


I don't think we need this to be an interface either. We should be able to get away with just offPeakWindow?: OffPeakWindow. If its specified, users probably want it to be enabled as well. In general I like to default to a single prop rather than specifying interfaces.

kaizencc · 2023-07-24T19:28:57Z

packages/aws-cdk-lib/aws-opensearchservice/lib/domain.ts

    }

+    let offPeakWindowOptions: OffPeakWindowOptions = {
+      enabled: props.offPeakWindowOptions?.enabled ?? true,


All domains created before this date have the off-peak window disabled by default.

So we can't just default to true here, as that will represent a behavior change to existing domains. Rather, without setting offPeakWindow (i.e. offPeakWindow: undefined) what we have to do is document the service-level behavior (enabled post 2/16/23, disabled prior). Then, we just pass undefined into the L1 and let the service handle the default.

Pull request has been modified.

kaizencc

Apologies @lpizzinidev I feel like I've been giving you half-baked reviews on this PR. But this last little redesign should be enough I think. Thanks for the quick turnarounds.

kaizencc · 2023-07-25T13:33:46Z

packages/aws-cdk-lib/aws-opensearchservice/lib/domain.ts

+   * You can't disable the off-peak window for a domain after it's enabled.
+   *
+   * @see https://docs.aws.amazon.com/it_it/AWSCloudFormation/latest/UserGuide/aws-properties-opensearchservice-domain-offpeakwindow.html
+   * @default - no off-peak window will be configured


Suggested change

* @default - no off-peak window will be configured

* @default - disabled for domains created before February 16, 2023. enabled for domains created after.

kaizencc · 2023-07-25T13:45:42Z

packages/aws-cdk-lib/aws-opensearchservice/lib/domain.ts

+      offPeakWindowOptions: props.offPeakWindow ? {
+        enabled: true,
+        offPeakWindow: {
+          windowStartTime: props.offPeakWindow.windowStartTime ?? {
+            hours: 22,
+            minutes: 0,
+          },
+        },
+      } : undefined,


so the reason why I'm not in love with this API yet is because to specify offPeakWindow with the given defaults we have to do:

offPeakWindow: {},

That's not very intuitive. Which now makes me understand why enabled was a prop in the first place :). Still, though, this potential experience to get the default window isn't great either:

offPeakWindow: { enabled: true, },

It feels weird to supply an object with just a boolean prop. So I suggest we do the following:

offPeakWindowEnabled: true, // if people want to customize the window // offPeakWindowStart: { // hours: 22, // minutes: 0, // },

I think having two separate properties on the base construct is the way to go and what we've generally done in the past for similar situations.

Of course, the default for offPeakWindowEnabled is true if offPeakWindowStart is set, false otherwise.

Pull request has been modified.

kaizencc

Thanks @lpizzinidev! Just added one last thing that allows offPeakWindowEnabled to be omitted if you specify offPeakWindowStart. Thanks for the contribution!

mergify · 2023-07-26T14:31:29Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Pull request has been modified.

mergify · 2023-07-27T18:16:37Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-07-27T18:43:04Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 16ba32f
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-07-27T18:43:24Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

feat(opensearchservice): added support for offPeakWindowOptions and s…

3a9943a

…oftwareUpdateOptions properties

aws-cdk-automation requested a review from a team July 18, 2023 12:18

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 star-contributor [Pilot] contributed between 25-49 PRs to the CDK labels Jul 18, 2023

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 18, 2023

kaizencc previously requested changes Jul 21, 2023

View reviewed changes

lpizzinidev added 2 commits July 22, 2023 12:11

Merge branch 'main' into awsgh-26388

3b24e02

handled default values

c667e52

lpizzinidev force-pushed the gh-26388 branch from c383dd4 to c667e52 Compare July 22, 2023 13:11

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 22, 2023

kaizencc previously requested changes Jul 24, 2023

View reviewed changes

lpizzinidev added 2 commits July 25, 2023 09:06

Merge branch 'main' into awsgh-26388

341b2d6

fixed properties interfaces and defaults

f81b682

fixed integration test

bb5f2a6

kaizencc previously requested changes Jul 25, 2023

View reviewed changes

fixed offPeakWindow property

e1e1cf3

users can specify just offpeakwindowstart

ef6b337

kaizencc changed the title ~~feat(opensearchservice): added support for offPeakWindowOptions and softwareUpdateOptions properties~~ feat(opensearchservice): L2 properties for offPeakWindowOptions and softwareUpdateOptions Jul 26, 2023

Merge branch 'main' into awsgh-26388

e75adf3

kaizencc previously approved these changes Jul 26, 2023

View reviewed changes

Merge branch 'main' into awsgh-26388

fbb4e20

kaizencc approved these changes Jul 27, 2023

View reviewed changes

Merge branch 'main' into awsgh-26388

16ba32f

mergify bot merged commit 02e8d58 into aws:main Jul 27, 2023

github-actions bot mentioned this pull request Aug 1, 2023

Monthly PRs metrics report - July 2023 #26583

Closed

	* @default - no off-peak window will be configured
	* @default - disabled for domains created before February 16, 2023. enabled for domains created after.

Conversation

lpizzinidev commented Jul 18, 2023 • edited by kaizencc Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kaizencc Jul 21, 2023

Choose a reason for hiding this comment

Uh oh!

lpizzinidev Jul 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

kaizencc Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

kaizencc Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

kaizencc Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

kaizencc Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

kaizencc Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Jul 26, 2023

Uh oh!

mergify bot commented Jul 27, 2023

Uh oh!

aws-cdk-automation commented Jul 27, 2023

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jul 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lpizzinidev commented Jul 18, 2023 •

edited by kaizencc

Loading

lpizzinidev Jul 22, 2023 •

edited

Loading