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

DATAGO-78232: Add new spring variable to override the use of user.home #189

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

gregmeldrum
Copy link
Collaborator

@gregmeldrum gregmeldrum commented Jun 27, 2024

What is the purpose of this change?

When running the docker image in k8s, the user.home get's set to '?' which causes the EMA to fail to load

How was this change implemented?

Anywhere that we use user.home, we add another variable that can be set on the command line (or via an env var) that overrides the use of user.home, so the "command" and "tfcommand" directories get created as expected.

I was able to test this in docker using the following command:

docker run -d -p 8180:8180 -v greg1.yml:/config/ema.yml --env JAVA_OPTS="-Dapp.commandroot=/opt" --name event-management-agent event-management-agent:ema-test1

The "magic" here is setting the environment variable JAVA_OPTS to be -Dapp.commandroot=/opt. This overrides the use of user.home. For the kubernetes deployment, we must add the JAVA_OPTS env var via sidekick.

How was this change tested?

Tested with jar from the command line and with docker container.
Tested with and without the new 'app.commandroot' variable set. With app.commandroot set, it as used as the root directory instead of the user's home directory.
Tested with config push and scan

Is there anything the reviewers should focus on/be aware of?

None

@gregmeldrum gregmeldrum changed the title Add a new spring variable that will override using the spring user.ho… DATAGO-78232: Add new spring variable to override the use of user.home Jun 27, 2024
Copy link

SonarQube Quality Gate

Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@@ -7,6 +7,6 @@
@Service
@Data
public class TerraformProperties {
@Value("${COMMAND_PATH:${user.home}${file.separator}tfcommands}")
@Value("${app.commandroot:${user.home}}${file.separator}tfcommands")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to update README instructions to include this new variable for those running EMA locally as a docker container?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This issue seems to be specific to the way we are deploying the container in k8s. I'm not sure if it's a rootless deployment or something, but this issue does not occur when running the docker container using the commands in the README. If you don't specify the app.commandroot Spring variable, it defaults to the previous behaviour of using the user.home as the root for the command and tfcommand directories so a user following instructions from our README will see no change.
If you'd like, I can add a section explaining how the new app.commandroot spring variable works and how they can use it to customize where the logs and terraform artifacts are created. Let me know and I can add it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, ok let's leave things this level of flexibility is not needed for local users. If some asks for it we can update the readme of how to do it.

@gregmeldrum gregmeldrum merged commit 8183c18 into main Jun 28, 2024
6 checks passed
@gregmeldrum gregmeldrum deleted the DATAGO-78232-fix-k8s-no-home-dir branch June 28, 2024 14:38
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