Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(opensearchservice): L2 properties for offPeakWindowOptions and softwareUpdateOptions #26403
feat(opensearchservice): L2 properties for offPeakWindowOptions and softwareUpdateOptions #26403
Changes from 3 commits
3a9943a
3b24e02
c667e52
341b2d6
f81b682
bb5f2a6
e1e1cf3
ef6b337
e75adf3
fbb4e20
16ba32f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 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.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 dont think we need an interface for this, lets default to a flatter API. Think we should just expose a boolean
enableAutoSoftwareUpdate
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.
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?
https://docs.aws.amazon.com/opensearch-service/latest/developerguide/off-peak.html
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 defaulted it to
00:00 UTC
https://docs.aws.amazon.com/opensearch-service/latest/developerguide/off-peak.html (in Enabling the off-peak window > CLI)
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.
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 passundefined
into the L1 and let the service handle the default.