-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Expose global retention settings via data stream lifecycle API #112210
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
Merged
elasticsearchmachine
merged 19 commits into
elastic:main
from
gmarouli:expose-global-retention-settings-v2
Sep 2, 2024
Merged
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
8442d90
checkpoint
gmarouli cce4f63
Add unit test
gmarouli 768d3ff
Update docs
gmarouli 1bc3240
Update docs/changelog/112210.yaml
gmarouli 6efbe9a
Update docs
gmarouli 87a1ee1
Merge branch 'main' into expose-global-retention-settings-v2
elasticmachine 0a717c1
Merge branch 'main' into expose-global-retention-settings-v2
elasticmachine 2dc7ed7
Merge branch 'main' into expose-global-retention-settings-v2
gmarouli 91e0b5b
Fix docs
gmarouli 9314259
Return an empty object for global retention
gmarouli 596f0b1
Fix typo
gmarouli 4a5bf0b
Fix test in tutorial-manage-new-data-stream.asciidoc
gmarouli 4dfa07f
Merge branch 'main' into expose-global-retention-settings-v2
gmarouli 15fa8e1
Fix accidentally skipped tests
gmarouli 71f15f6
Enable default lifecycle test
gmarouli 914a938
Merge and update tests
gmarouli 3e220d7
Merge branch 'main' into expose-global-retention-settings-v2
gmarouli 657798a
Merge branch 'main' into expose-global-retention-settings-v2
elasticmachine 5b6b6ed
Merge branch 'main' into expose-global-retention-settings-v2
elasticmachine File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
pr: 112210 | ||
summary: Expose global retention settings via data stream lifecycle API | ||
area: Data streams | ||
type: enhancement | ||
issues: [] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8 changes: 4 additions & 4 deletions
8
...eams/src/yamlRestTest/resources/rest-api-spec/test/data_stream/lifecycle/30_not_found.yml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
Do we not want to show empty global configuration if none is configured? I'm trying to think of something analogous in an API we have (to see if it does this, or shows empty) but haven't thought of anything yet.
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 have been going back and forth on this too. In the same when we have no data streams we show an empty list.
"data_streams": []
. But because this is an object and not a list, I wasn't sure how to approach it.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.
Do you think that having
global_retention: {}
is more descriptive? Like showing that, yes we do support it and it's non set?Uh oh!
There was an error while loading. Please reload this page.
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 changed the implementation to reflect this option.
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'm also on the fence. I have no strong feeling either way. But maybe it's nice having it always present -- that way you know it's not set, rather than that you're just not looking for it in the right place.
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.
Maybe @dakrone has an opinion or knows of some relevant precedent.
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.
One argument against this is that now we need to update every yaml rest test that returns a data stream to require the
data_stream_global_retention
, don't we? Otherwise backwards compatibility test runs will fail.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.
Yes, we do not have that many though. I will double check to ensure they are correct.
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.
While checking on the global retention in the existing get lifecycle tests, I noticed..... they were not even running :(. I fixed that in 15fa8e1.
I also decided to merge the two files into one. It seems to me that since the global retention is part of the response it should be part of the basic tests.