Remove default creds #138
Remove default creds #138
Conversation
Signed-off-by: Derek Ho <dxho@amazon.com>
derek-ho
requested review from
peterzhuamazon,
prudhvigodithi,
jackson-theisen and
phillbaker
as code owners
|
@peterzhuamazon @prudhvigodithi can you review this once? I am expecting CI to fail until after 2.12.0 docker image gets published, but looking for early feedback, or if you folks think this approach is appropriate - keep PR open until 2.12.0 image release, at which point CI would break and we merge in this PR to fix it. Not sure if I should modify provider_test.go. |
| @@ -45,7 +45,7 @@ func init() { | |||
|
|
|||
| opendistroOriginalConfigureFunc := testAccOpendistroProvider.ConfigureContextFunc | |||
| testAccOpendistroProvider.ConfigureContextFunc = func(c context.Context, d *schema.ResourceData) (interface{}, diag.Diagnostics) { | |||
| err := d.Set("url", "http://admin:admin@127.0.0.1:9200") | |||
| err := d.Set("url", "http://admin:myStrongPassword123!@127.0.0.1:9200") | |||
There was a problem hiding this comment.
Not sure if I should modify this/not sure how it's used - maybe in the integration test with opensearch 2.x?
There was a problem hiding this comment.
we can fetch this value from os if the tests are run in the docker container spun-up by the test workflow:
os.Getenv("OPENSEARCH_INITIAL_ADMIN_PASSWORD")
@prudhvigodithi correct me if I'm wrong here
| @@ -69,7 +69,7 @@ jobs: | |||
| key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }} | |||
| restore-keys: ${{ runner.os }}-go- | |||
| - name: Run Docker containers | |||
| run: docker-compose up --detach | |||
| run: docker-compose --env-file .github/workflows/.env up --detach | |||
| env: | |||
There was a problem hiding this comment.
@derek-ho @DarshitChanpura you can directly update here https://github.com/opensearch-project/terraform-provider-opensearch/blob/main/docker-compose.yml, no need for a new .github/workflows/.env. Also the main branch works for past releases of 2.x series, where its not required to add OPENSEARCH_INITIAL_ADMIN_PASSWORD. AFAIK it wont break but OPENSEARCH_INITIAL_ADMIN_PASSWORD would just redundant for old versions right?
There was a problem hiding this comment.
agreed. no need to modify this command. we can modify the docker-compose.yml directly if that's the file being used here to spin-up containers.
AFAIK it wont break but OPENSEARCH_INITIAL_ADMIN_PASSWORD would just redundant for old versions right?
Correct. This variable will only be used for 2.12 and above. Else it would simply not be used.
| - name: Dump docker logs on failure | ||
| if: failure() | ||
| uses: jwalton/gh-docker-logs@v2 | ||
| - name: Run the tests | ||
| run: | | ||
| export OPENSEARCH_URL=http://admin:admin@localhost:9200 | ||
| export OPENSEARCH_URL=http://admin:myStrongPassword123!@localhost:9200 |
There was a problem hiding this comment.
an alternative approach: we can define an env variable for this action and use that.
env:
ADMIN_PASSWORD: myStrongPassword123!
and use it as ${{ env.ADMIN_PASSWORD }}:
export OPENSEARCH_URL=http://admin:${{ env.ADMIN_PASSWORD }@localhost:9200
|
Converting this to draft since it will not pass CI until after 2.12.0 release. |
| @@ -0,0 +1 @@ | |||
| OPENSEARCH_INITIAL_ADMIN_PASSWORD=myStrongPassword123! | |||
There was a problem hiding this comment.
@derek-ho can you directly the env value in docker-compose file?
| @@ -91,13 +91,13 @@ jobs: | |||
| - name: Wait for Opensearch | |||
| # ensure that OS has come up and is available | |||
| run: | | |||
| ./script/wait-for-endpoint --timeout=20 http://admin:admin@localhost:9200 | |||
| ./script/wait-for-endpoint --timeout=20 http://admin:myStrongPassword123!@localhost:9200 | |||
There was a problem hiding this comment.
This will fail for versions below 2.12.0.
|
@prudhvigodithi Now that 2.12 is released this should be unblocked. Would you mind bringing this home? |
|
@prudhvigodithi I see you made a similar change recently, thanks for following up! I am closing this now since it is redundant. |
|
Thanks @derek-ho and @DarshitChanpura |
Description
Describe what this change achieves.
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.