-
Notifications
You must be signed in to change notification settings - Fork 58
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
Add spec for new rename_alias_pattern and rename_alias_replacement parameters to snapshot restore #615
base: main
Are you sure you want to change the base?
Add spec for new rename_alias_pattern and rename_alias_replacement parameters to snapshot restore #615
Conversation
The implementing PR for this spec change is not yet merged. |
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.
Looking good, needs a CHANGELOG, passing tests, etc.
tests/snapshot/snapshot/restore.yaml
Outdated
- path: /stories | ||
method: DELETE | ||
status: [200, 404] | ||
- path: /new_stories |
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.
We should call it something meaningful, maybe tales
?
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 mean new_stories
? This is the restored-with-new-name index. I changed it to renamed_stories
.
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 just don't love names like new_xyz
or xyz_1
because we plan to use these samples to generate documentation. It's better with names like "movies" or "films", so instead of "new_stories" maybe we can use something semantically interesting like "tales"?
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 name is for the rename parameters for this API. An entirely new name like that would not fit the most common use case of those parameters, which is to restore a copy of existing index with a new, non-conflicting name. So, even for documentation, I think a prefixed name like currently is more suitable. The current documentation of restore supports this - it has an example of a prefixed name, recovered-logs
, and a suffixed name, opendistro-reports-definitions_restored
, but no example of a entirely new name for renamed indexes.
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.
Understood, thanks. Maybe xyz-restored
would make more semantic sense?
Take this opportunity to look at these restore tests and see if better names than "my-xyz" can be used. Think of what you'd want to read in the docs. NBD, but much appreciated.
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 renamed this index and rewrote the rename parameters to match realistic use cases.
Changes AnalysisCommit SHA: fccd6a5 API ChangesSummary
ReportThe full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/11373857376/artifacts/2066074581 API Coverage
|
eadc2f8
to
b18145d
Compare
…rameters to snapshot restore Signed-off-by: Spencer G. Jones <[email protected]>
Signed-off-by: Spencer G. Jones <[email protected]>
Signed-off-by: Spencer G. Jones <[email protected]>
Signed-off-by: Spencer G. Jones <[email protected]>
fccd6a5
to
5bedafa
Compare
Signed-off-by: Spencer G. Jones <[email protected]>
This feature has been merged. |
Signed-off-by: Spencer G. Jones <[email protected]>
You'll need to update the 2.18 and 3.0 SHAs in CI as well for something that has the feature. I added some documentation for this in #642. |
@@ -359,6 +359,12 @@ components: | |||
type: string | |||
rename_replacement: | |||
type: string | |||
rename_alias_pattern: |
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.
Take the time to add a description:
field to these. Eventually they will appear in the documentation.
Description
Add spec for new rename_alias_pattern and rename_alias_replacement parameters to snapshot restore implemented in Opensearch PR 16292
Issues Resolved
No related issues
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.