[Build] deprecate renamed configurations#493
Merged
kavilla merged 1 commit intoopensearch-project:mainfrom Jun 22, 2021
Merged
Conversation
This was
linked to
issues
Jun 18, 2021
Member
Author
|
👍🏼 or 👎🏼 if this should go into this should definitely be backported into |
Collaborator
|
✅ DCO Check Passed 45057bd |
ananzh
reviewed
Jun 18, 2021
tmarkley
reviewed
Jun 18, 2021
Contributor
tmarkley
left a comment
There was a problem hiding this comment.
Can we add tests for the config changes?
45057bd to
647d98c
Compare
Collaborator
|
✅ DCO Check Passed 647d98c |
647d98c to
134686c
Compare
Collaborator
|
✅ DCO Check Passed 134686c |
While renaming after the fork, configurations were renamed and replaced with keywords related to OpenSearch. This meant that anyone who migrated to OpenSearch Dashboards who had configured their YAML file no longer were able to carry over those changes and run the application. This prevented the application from starting due to unknown config keys. Although, this still does not allow the application to work out of the box because people will need to make sure then rename their kibana.yml to opensearch_dashboards.yml, but once they do they do not need to modify the content of the config. Added unit tests to test on the server configs. Issues resolved: opensearch-project#440 Partially resolves: opensearch-project#334 Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
134686c to
25eb06f
Compare
Collaborator
|
✅ DCO Check Passed 25eb06f |
Member
Author
Added the best I can, specifically related to the core server configs. |
Member
Author
|
Re-running tests, full suite should be done in an hour. |
Member
Author
|
Tests all passing on the CI. |
boktorbb
reviewed
Jun 21, 2021
Comment on lines
+138
to
+155
| const deprecations: ConfigDeprecationProvider = ({ renameFromRoot }) => [ | ||
| renameFromRoot('elasticsearch.sniffOnStart', 'opensearch.sniffOnStart'), | ||
| renameFromRoot('elasticsearch.sniffInterval', 'opensearch.sniffInterval'), | ||
| renameFromRoot('elasticsearch.sniffOnConnectionFault', 'opensearch.sniffOnConnectionFault'), | ||
| renameFromRoot('elasticsearch.hosts', 'opensearch.hosts'), | ||
| renameFromRoot('elasticsearch.username', 'opensearch.username'), | ||
| renameFromRoot('elasticsearch.password', 'opensearch.password'), | ||
| renameFromRoot('elasticsearch.requestHeadersWhitelist', 'opensearch.requestHeadersWhitelist'), | ||
| renameFromRoot('elasticsearch.customHeaders', 'opensearch.customHeaders'), | ||
| renameFromRoot('elasticsearch.shardTimeout', 'opensearch.shardTimeout'), | ||
| renameFromRoot('elasticsearch.requestTimeout', 'opensearch.requestTimeout'), | ||
| renameFromRoot('elasticsearch.pingTimeout', 'opensearch.pingTimeout'), | ||
| renameFromRoot('elasticsearch.logQueries', 'opensearch.logQueries'), | ||
| renameFromRoot('elasticsearch.optimizedHealthcheckId', 'opensearch.optimizedHealthcheckId'), | ||
| renameFromRoot('elasticsearch.ssl', 'opensearch.ssl'), | ||
| renameFromRoot('elasticsearch.apiVersion', 'opensearch.apiVersion'), | ||
| renameFromRoot('elasticsearch.healthCheck', 'opensearch.healthCheck'), | ||
| renameFromRoot('elasticsearch.ignoreVersionMismatch', 'opensearch.ignoreVersionMismatch'), |
Contributor
There was a problem hiding this comment.
Will these need to be updated any time we introduce a new setting?
Member
Author
There was a problem hiding this comment.
When a new setting is added you dont need to add it here, I added your optimizedHealthcheckId just thinking there are so many configs for the engine that somebody might accidentally add the setting here. I can remove it though since it is completely new. Let me know what you think.
tmarkley
approved these changes
Jun 21, 2021
ananzh
approved these changes
Jun 22, 2021
kavilla
added a commit
that referenced
this pull request
Jun 22, 2021
While renaming after the fork, configurations were renamed and replaced with keywords related to OpenSearch. This meant that anyone who migrated to OpenSearch Dashboards who had configured their YAML file no longer were able to carry over those changes and run the application. This prevented the application from starting due to unknown config keys. Although, this still does not allow the application to work out of the box because people will need to make sure then rename their kibana.yml to opensearch_dashboards.yml, but once they do they do not need to modify the content of the config. Added unit tests to test on the server configs. Issues resolved: #440 Partially resolves: #334 Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
kavilla
added a commit
that referenced
this pull request
Jun 22, 2021
While renaming after the fork, configurations were renamed and replaced with keywords related to OpenSearch. This meant that anyone who migrated to OpenSearch Dashboards who had configured their YAML file no longer were able to carry over those changes and run the application. This prevented the application from starting due to unknown config keys. Although, this still does not allow the application to work out of the box because people will need to make sure then rename their kibana.yml to opensearch_dashboards.yml, but once they do they do not need to modify the content of the config. Added unit tests to test on the server configs. Issues resolved: #440 Partially resolves: #334 Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Description
While renaming after the fork, configurations were renamed
and replaced with keywords related to OpenSearch.
This meant that anyone who migrated to OpenSearch Dashboards
who had configured their YAML file no longer were able to
carry over those changes and run the application. This
prevented the application from starting due to unknown config
keys. Although, this still does not allow the application to
work out of the box because people will need to make sure
then rename their kibana.yml to opensearch_dashboards.yml,
but once they do they do not need to modify the content
of the config.
Signed-off-by: Kawika Avilla kavilla414@gmail.com
Issues Resolved
#440
Partial: #334
Check List