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

Remove references to the default credentials of admin:admin #449

Merged
merged 10 commits into from
Mar 12, 2024

Conversation

derek-ho
Copy link
Contributor

@derek-ho derek-ho commented Dec 18, 2023

Description

Remove references to the default credentials of admin:admin and changes the CI according to default admin credentials changes for the 2.12.0 release.

Issues Resolved

List any issues this PR will resolve, e.g. Closes [...].

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 18, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.26%. Comparing base (ee43c33) to head (198261c).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #449   +/-   ##
=======================================
  Coverage   57.26%   57.26%           
=======================================
  Files         315      315           
  Lines        9823     9823           
=======================================
  Hits         5625     5625           
  Misses       2904     2904           
  Partials     1294     1294           
Flag Coverage Δ
integration 50.80% <ø> (ø)
unit 12.84% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

guides/index_lifecycle.md Outdated Show resolved Hide resolved
@derek-ho
Copy link
Contributor Author

@dblock can you take another look here? I am not sure why the tests against the latest secure cluster are passing, since the password changes should not be there? I also think the unit test failure is unrelated?

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.

Is this change necessary at all until we update CI/CD to 2.12? Or is it because we test against latest?

ENV password="admin"
RUN if [ "$OPENSEARCH_VERSION" = "latest" ]; then \
export password="myStrongPassword123!"; \
fi
Copy link
Member

Choose a reason for hiding this comment

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

This will break immediately after 2.12 is released.

Copy link
Member

@DarshitChanpura DarshitChanpura Jan 17, 2024

Choose a reason for hiding this comment

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

@derek-ho this should be updated to check for versions 2.12 and above

@derek-ho
Copy link
Contributor Author

This change is not necessary until after release, seems like overall CI is healthy. Marking this until draft until after the release.

@derek-ho derek-ho marked this pull request as draft January 18, 2024 15:51
@DarshitChanpura
Copy link
Member

@dblock @VijayanB @VachaShah @Jakob3xD Now that 2.12 is released this should be unblocked. Would one of you mind bringing this home?

@VachaShah
Copy link
Collaborator

@dblock @VijayanB @VachaShah @Jakob3xD Now that 2.12 is released this should be unblocked. Would one of you mind bringing this home?

Sure! @derek-ho Is this ready for review now?

@derek-ho derek-ho marked this pull request as ready for review February 27, 2024 16:59
@derek-ho
Copy link
Contributor Author

derek-ho commented Mar 7, 2024

@Jakob3xD thanks. I'm getting a little stuck on this PR since it seems by the time we reach here we have no context in regards to the OpenSearch version, since that is spun up as a different step. Any ideas?

derek-ho and others added 10 commits March 11, 2024 12:04
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>
Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>
@Jakob3xD
Copy link
Collaborator

@Jakob3xD thanks. I'm getting a little stuck on this PR since it seems by the time we reach here we have no context in regards to the OpenSearch version, since that is spun up as a different step. Any ideas?

I hope it is okay that I force pushed into your branch. I adjusted the docker file to fix the health check and change the go security check to find the password by trying them out. Otherwise we need to adjust the workflow to export the opensearch version as env so we can parse it as you tried before.

@Jakob3xD Jakob3xD requested a review from dblock March 11, 2024 14:16
@dblock dblock merged commit acce269 into opensearch-project:main Mar 12, 2024
52 checks passed
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.

5 participants