-
Notifications
You must be signed in to change notification settings - Fork 277
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
Updates Docker and windows distribution build files to reflect to changes in admin user setup #4274
Updates Docker and windows distribution build files to reflect to changes in admin user setup #4274
Conversation
… user setup Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4274 +/- ##
=======================================
Coverage 91.35% 91.35%
=======================================
Files 190 190
Lines 6175 6175
=======================================
Hits 5641 5641
Misses 534 534 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
@prudhvigodithi @rishabh6788 Mind reviewing this? |
…onment variable Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
@@ -35,6 +36,7 @@ services: | |||
- cluster.initial_cluster_manager_nodes=opensearch-node1,opensearch-node2 | |||
- bootstrap.memory_lock=true | |||
- OPENSEARCH_JAVA_OPTS=-Xms512m -Xmx512m | |||
- OPENSEARCH_INITIAL_ADMIN_PASSWORD=${OPENSEARCH_INITIAL_ADMIN_PASSWORD} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these changes the default docker-compose files for 2.x line to now include an additional env variable (to be passed by user) to set the admin password.
This will be accompanied by the change in download instructions on the project-website (file to change) to reflect the new requirement as an additional step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add a comment for this line, so that people knows it is only for 2.12.0 and up.
Also, the documentation and the download page needs to be tweaked with the same changes as well.
https://opensearch.org/docs/2.11/install-and-configure/install-opensearch/docker/
https://opensearch.org/downloads.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @DarshitChanpura please take a look at this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it above, and following the convention, I didn't add it second time as no other settings that have comments on the first one have comments here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the comment to mention that it is required for 2.12 and higher
@peterzhuamazon @prudhvigodithi @rishabh6788 Can I please get more reviews? |
@peterzhuamazon @prudhvigodithi @rishabh6788 Could I please get reviews on this? We suspect this is currently causing integ tests to fail, and merging this will unblock them. |
Need to change these as well if you plan to change docker-compose: |
Also need @prudhvigodithi to confirm on @DarshitChanpura comment above regarding keeping the exit 1 for docker and tar, but remove for deb and rpm. |
Yes, I have PRs out to to address both: |
@peterzhuamazon RPM and DEB setup already has |
@peterzhuamazon Could I get re-review on this PR? |
The same #4250 (comment) goes here as well. |
@DarshitChanpura can you please use as
|
Signed-off-by: Darshit Chanpura <[email protected]>
Similar to #4250 (comment), I've updated docker related scripts with printf. There is no echo -e or printf for windows from what I understand |
@peterzhuamazon @prudhvigodithi @rishabh6788 Could I get some more reviews? |
Signed-off-by: Darshit Chanpura <[email protected]>
@peterzhuamazon Is there anything blocking this PR still? |
CI task seemed to have failed due to connectivity issues, and re-run should solve it.
|
@peterzhuamazon @rishabh6788 Could I get some more reviews? |
Signed-off-by: Darshit Chanpura <[email protected]>
@peterzhuamazon Could I get your reviews on this? Also could you or a maintainer trigger re-run for Lint Checker as it timed out during the latest run. |
Description
As a follow-up of #4250, this PR updates docker and windows distribution install script to adapt to changes in demo configuration setup.
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.