Skip to content

Conversation

nokados
Copy link

@nokados nokados commented Dec 27, 2024

Description

It fixes the same problem as in #267 in the same way, but this time it was caused by fetch_opensearch_repo

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.

Copy link

codecov bot commented Dec 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.39%. Comparing base (ba715b9) to head (da6d388).
Report is 90 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #879      +/-   ##
==========================================
- Coverage   71.95%   70.39%   -1.56%     
==========================================
  Files          91      125      +34     
  Lines        8001     9290    +1289     
==========================================
+ Hits         5757     6540     +783     
- Misses       2244     2750     +506     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

There are two failing test suites with this change, care to take a look?

@nokados nokados requested a review from dblock December 31, 2024 13:14
@dblock
Copy link
Member

dblock commented Dec 31, 2024

Hmmm, this seems like a workaround around a workaround. I see a lot of code like this

Feels like a fix should be somewhere deeper.

What are the state(s) of your environment variables OPENSEARCH_INITIAL_ADMIN_PASSWORD and OPENSEARCH_PASSWORD that causes a local run to fail?

@nokados
Copy link
Author

nokados commented Jan 2, 2025

To reproduce the issue, just follow the DEVELOPER_GUIDE.

So, in the local environment, OPENSEARCH_PASSWORD may be set. OPENSEARCH_INITIAL_ADMIN_PASSWORD is for integration tests.

I’m currently unable to dedicate time to explore a deeper solution. At the moment, the code isn’t working as intended. If the extra two lines in the tests feel unnecessary to you, you’re welcome to refactor it, but unfortunately, I won’t be able to take that on right now.

@dblock
Copy link
Member

dblock commented Jan 2, 2025

Thanks for your help so far @nokados.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants