Skip to content
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

Fixing error: adding a new/forgotten parameter to the configuration for checking the config on startup in plugins/repository-s3 #7924

Merged
merged 3 commits into from
Jun 6, 2023

Conversation

nice-redbull
Copy link
Contributor

@nice-redbull nice-redbull commented Jun 6, 2023

Description

Fixing error: adding a new/forgotten parameter to the configuration for checking the config on startup.

The error fixes the situation that arises when configuring a proxy for accessing the S3 service. When setting up the proxy, the proxy.type is required, but this parameter causes an error upon node startup. This change rectifies the current situation and ensures behavior as described in the documentation.

This modification has already been tested on our systems.

Related Issues

#2160

s3.client.default.proxy.type
repository-s3

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • [?] Commits are signed per the DCO using --signoff
  • [ x ] Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

Fixing error: adding a new parameter to the configuration for checking the config on startup.

Signed-off-by: Konstantin Gerasimenko <[email protected]>
@nice-redbull
Copy link
Contributor Author

@willyborankin

Copy link
Collaborator

@reta reta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nice-redbull could you please:

  • add CHANGELOG.md entry (under Unreleased 2.x)
  • add test case for the issue

@nice-redbull nice-redbull changed the title Fixing error: adding a new/forgotten parameter to the configuration for checking the config on startup in plugin/repository-s3 Fixing error: adding a new/forgotten parameter to the configuration for checking the config on startup in plugins/repository-s3 Jun 6, 2023
@nice-redbull
Copy link
Contributor Author

@nice-redbull could you please:

* add CHANGELOG.md entry (under `Unreleased 2.x`)

ok

* add test case for the issue

I have no idea how to do it :-) maybe a @willyborankin will help?

Signed-off-by: Konstantin Gerasimenko <[email protected]>
@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.search.backpressure.SearchBackpressureIT.testSearchTaskCancellationWithHighCpu

@codecov
Copy link

codecov bot commented Jun 6, 2023

Codecov Report

Merging #7924 (2afc0a9) into main (8dd5461) will increase coverage by 0.07%.
The diff coverage is n/a.

❗ Current head 2afc0a9 differs from pull request most recent head eedf4e7. Consider uploading reports for the commit eedf4e7 to get more accurate results

@@             Coverage Diff              @@
##               main    #7924      +/-   ##
============================================
+ Coverage     70.75%   70.82%   +0.07%     
- Complexity    56236    56245       +9     
============================================
  Files          4689     4689              
  Lines        266324   266324              
  Branches      39087    39087              
============================================
+ Hits         188425   188626     +201     
+ Misses        61949    61668     -281     
- Partials      15950    16030      +80     
Impacted Files Coverage Δ
...opensearch/repositories/s3/S3RepositoryPlugin.java 0.00% <ø> (ø)

... and 461 files with indirect coverage changes

@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.search.backpressure.SearchBackpressureIT.testSearchTaskCancellationWithHighCpu
      1 org.opensearch.remotestore.RemoteStoreRefreshListenerIT.testRemoteRefreshRetryOnFailure

@reta reta merged commit d55813e into opensearch-project:main Jun 6, 2023
@reta reta added bug Something isn't working backport 2.x Backport to 2.x branch labels Jun 6, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 6, 2023
…or checking the config on startup in plugins/repository-s3 (#7924)

* Update S3RepositoryPlugin.java

Fixing error: adding a new parameter to the configuration for checking the config on startup.

Signed-off-by: Konstantin Gerasimenko <[email protected]>

* Update CHANGELOG.md

Signed-off-by: Konstantin Gerasimenko <[email protected]>

* Add proxy settings type to test case settings to make sure it is accepted

Signed-off-by: Andriy Redko <[email protected]>

---------

Signed-off-by: Konstantin Gerasimenko <[email protected]>
Signed-off-by: Andriy Redko <[email protected]>
Co-authored-by: Andriy Redko <[email protected]>
(cherry picked from commit d55813e)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
reta added a commit that referenced this pull request Jun 6, 2023
…or checking the config on startup in plugins/repository-s3 (#7924) (#7929)

* Update S3RepositoryPlugin.java

Fixing error: adding a new parameter to the configuration for checking the config on startup.



* Update CHANGELOG.md



* Add proxy settings type to test case settings to make sure it is accepted



---------




(cherry picked from commit d55813e)

Signed-off-by: Konstantin Gerasimenko <[email protected]>
Signed-off-by: Andriy Redko <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Andriy Redko <[email protected]>
@dblock
Copy link
Member

dblock commented Jun 6, 2023

This could use a test @nice-redbull, maybe try writing one? The initial problem states that when setting up the proxy, you should be getting a "the proxy.type is required" error. If I undo this change no tests fail, so we can easily re-introduce a regression if we're not careful.

@reta
Copy link
Collaborator

reta commented Jun 6, 2023

This could use a test @nice-redbull, maybe try writing one? The initial problem states that when setting up the proxy, you should be getting a "the proxy.type is required" error. If I undo this change no tests fail, so we can easily re-introduce a regression if we're not careful.

@dblock I have added proxy to test settings here [1] - the tests are failing on main since the proxy type was not a recognizable property.

[1] https://github.com/opensearch-project/OpenSearch/pull/7924/files#diff-0e02bc404b49fa24e689c323473778abf40549bbacd9473ac0c192331e53bf0dR143

@dblock
Copy link
Member

dblock commented Jun 6, 2023

Aha I see, thanks @reta!

sandeshkr419 pushed a commit to sandeshkr419/OpenSearch that referenced this pull request Jun 8, 2023
…or checking the config on startup in plugins/repository-s3 (opensearch-project#7924)

* Update S3RepositoryPlugin.java

Fixing error: adding a new parameter to the configuration for checking the config on startup.

Signed-off-by: Konstantin Gerasimenko <[email protected]>

* Update CHANGELOG.md

Signed-off-by: Konstantin Gerasimenko <[email protected]>

* Add proxy settings type to test case settings to make sure it is accepted

Signed-off-by: Andriy Redko <[email protected]>

---------

Signed-off-by: Konstantin Gerasimenko <[email protected]>
Signed-off-by: Andriy Redko <[email protected]>
Co-authored-by: Andriy Redko <[email protected]>
gaiksaya pushed a commit to gaiksaya/OpenSearch that referenced this pull request Jun 26, 2023
…or checking the config on startup in plugins/repository-s3 (opensearch-project#7924) (opensearch-project#7929)

* Update S3RepositoryPlugin.java

Fixing error: adding a new parameter to the configuration for checking the config on startup.



* Update CHANGELOG.md



* Add proxy settings type to test case settings to make sure it is accepted



---------




(cherry picked from commit d55813e)

Signed-off-by: Konstantin Gerasimenko <[email protected]>
Signed-off-by: Andriy Redko <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Andriy Redko <[email protected]>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
…or checking the config on startup in plugins/repository-s3 (opensearch-project#7924)

* Update S3RepositoryPlugin.java

Fixing error: adding a new parameter to the configuration for checking the config on startup.

Signed-off-by: Konstantin Gerasimenko <[email protected]>

* Update CHANGELOG.md

Signed-off-by: Konstantin Gerasimenko <[email protected]>

* Add proxy settings type to test case settings to make sure it is accepted

Signed-off-by: Andriy Redko <[email protected]>

---------

Signed-off-by: Konstantin Gerasimenko <[email protected]>
Signed-off-by: Andriy Redko <[email protected]>
Co-authored-by: Andriy Redko <[email protected]>
Signed-off-by: Shivansh Arora <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants