Expand snapshot repository types #2255
Conversation
Use a different type definition for each repository type since differences are significant.
| settings: SourceOnlyRepositorySettings | ||
| } | ||
|
|
||
| export type RepositorySettings = |
There was a problem hiding this comment.
Seems like we don't need this type.
There was a problem hiding this comment.
It gets imported and used here. I'm not sure why the request body accepts both repository and settings keys when repository.settings also exists. 🤷
There was a problem hiding this comment.
Hmm... I'll look at the ES source code to understand if this is needed. Now @variants internal tag='type' won't work here since the union members don't have a type property.
There was a problem hiding this comment.
Good catch on the type property. Fixed that.
Hmm... I'll look at the ES source code to understand if this is needed.
I am still not great at navigating the Java codebase, but if this is the request, it certainly does not seem like it supports an optional repository property. I scrolled through a lot of the repository creation tests, too, and didn't see it. Feel free to correct my reading of it if I'm totally in the wrong place.
There was a problem hiding this comment.
We have to look a the parsing or the request body - the request just has type and settings. So it's actually a plain Repository object.
And when we look at the docs there's no type attribute inside the settings.
So we need to remove the type field in all repository settings, and also this RepositorySettings enumeration.
And SnapshotRepositoryCreateRequest should be updated to have body: Repository.
Once this is done, we're good to go!
So we can get a repository's uuid without having to cast to its exact type.
|
For interface SnapshotRepositorySettings , it has read_only and readonly, any way we can make it clearer what each individual behave? |
|
@DesAWSume |
|
@DesAWSume I did discover JSDoc has an
|
|
@swallez could I get a final review on this? only outstanding thing is this concern, but it was already in there like this so maybe we can address separately. |
| settings: SourceOnlyRepositorySettings | ||
| } | ||
|
|
||
| export type RepositorySettings = |
There was a problem hiding this comment.
We have to look a the parsing or the request body - the request just has type and settings. So it's actually a plain Repository object.
And when we look at the docs there's no type attribute inside the settings.
So we need to remove the type field in all repository settings, and also this RepositorySettings enumeration.
And SnapshotRepositoryCreateRequest should be updated to have body: Repository.
Once this is done, we're good to go!
|
@swallez One month and a parental leave later... I think I got the last of this sorted out. 😆 Please take a look and let me know if I addressed what you were saying properly. |
|
Following you can find the validation results for the APIs you have changed.
You can validate these APIs yourself by using the |
There was a problem hiding this comment.
@JoshMock I added a few commits:
- fix a bug/limitation of compiler validations so that conflicting identifiers check include the
codegen_nameannotation... - ... which allows to use
repositoryinstead ofrepository_settingsfor the body name (repositorywas used in the original code, this will avoid breaking changes in some languages) - removed the
typeattribute in settings class - only kept
readonlyand removedread_onlyas I couldn't findread_onlyin the source code or in the docs.
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-7.17 7.17
# Navigate to the new working tree
cd .worktrees/backport-7.17
# Create a new branch
git switch --create backport-2255-to-7.17
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 c45e69e81dc10e95176638cb415c2c3143faccfa
# Push it to GitHub
git push --set-upstream origin backport-2255-to-7.17
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-7.17Then, create a pull request where the |
(cherry picked from commit c45e69e)
|
thanks so much @swallez! ❤️ |
**Announcement**
* A long awaited feature has been added, stealthily. It's fully in the documentation, but I do
not yet plan to make a big announcement about it. In actions that search through indices, you
can now specify a ``search_pattern`` to limit the number of indices that will be filtered. If
no search pattern is specified, the behavior will be the same as it ever was: it will search
through ``_all`` indices. The actions that support this option are: allocation, close,
cold2frozen, delete_indices, forcemerge, index_settings, open, replicas, shrink, and snapshot.
**Bugfix**
* A mixup with naming conventions from the PII redacter tool got in the way of the cold2frozen
action completing properly.
**Changes**
* Version bump: ``es_client==8.13.0``
* With the version bump to ``es_client`` comes a necessary change to calls to create a
repository. In elastic/elasticsearch-specification#2255 it became
clear that using ``type`` and ``settings`` as it has been was insufficient for repository
settings, so we go back to using a request ``body`` as in older times. This change affects
``esrepomgr`` in one place, and otherwise only in snapshot/restore testing.
* Added the curator.helpers.getters.meta_getter to reduce near duplicate functions.
* Changed curator.helpers.getters.get_indices to use the _cat API to pull indices. The primary
driver for this is that it avoids pulling in the full mapping and index settings when all we
really need to return is a list of index names. This should help keep memory from ballooning
quite as much. The function also now allows for a search_pattern kwarg to search only for
indices matching a pattern. This will also potentially make the initial index return list much
smaller, and the list of indices needing to be filtered that much smaller.
* Tests were added to ensure that the changes for ``get_indices`` work everywhere.
* Tests were added to ensure that the new ``search_pattern`` did not break anything, and does
behave as expected.
* WIP branch for debugging cold2frozen weirdness
* Release prep for 8.0.14
**Announcement**
* A long awaited feature has been added, stealthily. It's fully in the documentation, but I do
not yet plan to make a big announcement about it. In actions that search through indices, you
can now specify a ``search_pattern`` to limit the number of indices that will be filtered. If
no search pattern is specified, the behavior will be the same as it ever was: it will search
through ``_all`` indices. The actions that support this option are: allocation, close,
cold2frozen, delete_indices, forcemerge, index_settings, open, replicas, shrink, and snapshot.
**Bugfix**
* A mixup with naming conventions from the PII redacter tool got in the way of the cold2frozen
action completing properly.
**Changes**
* Version bump: ``es_client==8.13.0``
* With the version bump to ``es_client`` comes a necessary change to calls to create a
repository. In elastic/elasticsearch-specification#2255 it became
clear that using ``type`` and ``settings`` as it has been was insufficient for repository
settings, so we go back to using a request ``body`` as in older times. This change affects
``esrepomgr`` in one place, and otherwise only in snapshot/restore testing.
* Added the curator.helpers.getters.meta_getter to reduce near duplicate functions.
* Changed curator.helpers.getters.get_indices to use the _cat API to pull indices. The primary
driver for this is that it avoids pulling in the full mapping and index settings when all we
really need to return is a list of index names. This should help keep memory from ballooning
quite as much. The function also now allows for a search_pattern kwarg to search only for
indices matching a pattern. This will also potentially make the initial index return list much
smaller, and the list of indices needing to be filtered that much smaller.
* Tests were added to ensure that the changes for ``get_indices`` work everywhere.
* Tests were added to ensure that the new ``search_pattern`` did not break anything, and does
behave as expected.
Use a different type definition for each repository type since differences are significant.
A community member noted that some types were missing for S3 snapshot repositories, and inspecting the spec made it clear that it was an issue beyond just S3.