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

Merge in keystore_password in target server.env if present #1097

Merged
merged 2 commits into from
Feb 25, 2021

Conversation

scottkurz
Copy link
Member

@scottkurz scottkurz commented Feb 25, 2021

Signed-off-by: Scott Kurz skurz@us.ibm.com

Fixes #1096

Note the change is a no-op if you have either a server.env in configDir or a serverEnvFile parm configured.

Manual test checklist

  • env mvn prop - basic (generated keystore_password gets merged)
  • env mvn prop including keystore_password (takes precedence over one generated on server create)
  • env mvn prop + server.env in configDir (generated keystore_password lost, env from mvn props)
  • env mvn prop + -DserverEnvFile (generated keystore_password lost, env from mvn props)
  • env mvn prop - basic with target/liberty/wlp/usr/servers/defaultServer/server.env already in place from previous execution WITHOUT keystore_password (just the env props are written to server.env..no error)

Signed-off-by: Scott Kurz <skurz@us.ibm.com>
Copy link
Member

@cherylking cherylking left a comment

Choose a reason for hiding this comment

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

Overall it looks good. I just wonder if we need some consensus that this change is appropriate (YK, Eric, etc.). Also, if we make this change here, then we need to update Gradle the same way.

@scottkurz
Copy link
Member Author

Thx for the review @cherylking .. will wait to get more opinions: @yeekangc @ericglau if this change seems OK.

Copy link
Contributor

@ericglau ericglau left a comment

Choose a reason for hiding this comment

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

Just one minor comment. Overall it looks fine to me.

docs/common-server-parameters.md Outdated Show resolved Hide resolved
@scottkurz
Copy link
Member Author

Also, if we make this change here, then we need to update Gradle the same way.

@cherylking I think it'd be OK to fix this in Maven, merge, open a ci.gradle issue but fix later. I don't think anyone would soon complain about this as an inconsistency across Maven vs. Gradle. I know .. there's always the risk that it just sits there and doesn't get done. But what do you think?

Signed-off-by: Scott Kurz <skurz@us.ibm.com>
Copy link
Member

@cherylking cherylking left a comment

Choose a reason for hiding this comment

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

The changes look good. Please open a ci.gradle issue to implement this same functionality. Also, can you open a ci.maven issue to add tests for this? I see you did manual tests, but we should have a couple integration tests for this.

@scottkurz
Copy link
Member Author

scottkurz commented Feb 25, 2021

Gradle issue: OpenLiberty/ci.gradle#587
ITs: #1098

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.

Env variables from Maven properties overwrite generated keystore password
3 participants