Skip to content

Conversation

@kpalang
Copy link
Contributor

@kpalang kpalang commented Jun 27, 2025

Referencing OpenIntegrationEngine/engine#40 and taking inspiration from OpenIntegrationEngine/engine#126.

The PR creates the Dockerfiles required for official runtime Docker images.

@kpalang kpalang requested a review from jonbartels June 27, 2025 17:27
@kpalang kpalang force-pushed the Issue-40-docker-image-for-releases branch from b2c7c86 to dc97fcc Compare June 27, 2025 17:31
@kpalang
Copy link
Contributor Author

kpalang commented Jun 27, 2025

Still have to figure out nextgenhealthcare/connect-docker#42

@mgaffigan
Copy link

Still have to figure out nextgenhealthcare/connect-docker#42

sed with doubling?

echo $VMOPTIONS | sed -E '
  # Remap doubled commas to \x01
  s/,,/\x01/g;

  # Break into lines by unescaped commas
  s/,/\n/g;

  # Restore the escaped commas
  s/\x01/,/g;
' >> "$APP_DIR/oieserver.vmoptions"

@kpalang
Copy link
Contributor Author

kpalang commented Jun 27, 2025

sed with doubling?

Since we now have full control over the code, it doesn't have to follow the old pattern. There is, of course, desirability in having the most backwards compatibility.

I'm even thinking of defining a new optional env to read values with a different delimiter, | for example.Something visually easy to type, but also retaining as much compatibility as possible...

@kpalang kpalang requested review from a team, gibson9583, kayyagari, pacmano1, ssrowe and tonygermano and removed request for a team June 27, 2025 19:55
@mgaffigan
Copy link

mgaffigan commented Jun 27, 2025

Since we now have full control over the code, it doesn't have to follow the old pattern. There is, of course, desirability in having the most backwards compatibility.

Fair. Not sure how fancy this needs to get. File/config mounts can easily be used to replace custom.vmoptions in docker and k8s. I don't think that's as much an option for mirth.properties since it doesn't seem to support includes. Perhaps adding includes to mirth.properties is an option? That would also simplify the install/upgrade story.

Edit:
Actually, I misspoke. mirth.properties does seem to support includes. Can we just make this a mount? Less magic. I understand the envs for the database and common properties, but at some point, we're re-inventing config mounts.

kayyagari
kayyagari previously approved these changes Jun 28, 2025
@jonbartels
Copy link
Contributor

I tested these builds by running the compose locally. It works. OIE launches and I can log in.

The README was largely dervied from NG, it says how to actually USE the software. I also grabbed the examples directory and updated it to our image

@jonbartels jonbartels requested a review from leovander June 30, 2025 15:11
jonbartels
jonbartels previously approved these changes Jun 30, 2025
kayyagari
kayyagari previously approved these changes Jun 30, 2025
gibson9583
gibson9583 previously approved these changes Jun 30, 2025
@kpalang
Copy link
Contributor Author

kpalang commented Jul 1, 2025

Edit: Actually, I misspoke. mirth.properties does seem to support includes. Can we just make this a mount? Less magic. I understand the envs for the database and common properties, but at some point, we're re-inventing config mounts.

Good point! I shall Investigate this a little.

@kpalang
Copy link
Contributor Author

kpalang commented Jul 1, 2025

Actually, question for @OpenIntegrationEngine/maintainers, should we break compat by dropping the VMOPTIONS env and forcing everything into conf/custom.vmoptions? K8s, Docker, and Nomad orchestrators all support mounting configurations into files, and native deployments can simply write to that file anyway.

@kpalang
Copy link
Contributor Author

kpalang commented Jul 1, 2025

@mgaffigan regarding the includes keyword from commons-configuration2, I think it's worth investigating, but in within a separate issue/pr.
This is because the current implementation in engine doesn't handle duplicates - it simply returns a List with the two values, which most likely breaks other fetches in the codebase.

Given mirth.propertiesand more.properties, serverProperties.getProperty("key2") returns an ArrayList of ["value2", "value2 but some more"].

# mirth.properties
key1 = "value1"
key2 = "value2"

include = more.properties
# more.properties
key2 = "value2 but some more"
key3 = "value3"

@kpalang kpalang dismissed stale reviews from gibson9583, kayyagari, and jonbartels via 328f6fa July 1, 2025 10:53
@jonbartels
Copy link
Contributor

Actually, question for @OpenIntegrationEngine/maintainers, should we break compat by dropping the VMOPTIONS env and forcing everything into conf/custom.vmoptions? K8s, Docker, and Nomad orchestrators all support mounting configurations into files, and native deployments can simply write to that file anyway.

NO. Using includes and vmoptions is a good idea, but this should be a separate PR in a later issue so we can consider the compat issues.

@tonygermano
Copy link
Member

tonygermano commented Jul 1, 2025

Actually, question for @OpenIntegrationEngine/maintainers, should we break compat by dropping the VMOPTIONS env and forcing everything into conf/custom.vmoptions? K8s, Docker, and Nomad orchestrators all support mounting configurations into files, and native deployments can simply write to that file anyway.

I don't think we should break it either. Keeping the VMOPTIONS env var does not prevent someone from using conf/custom.vmoptions instead, whether they choose to or are required to because of the current VMOPTIONS behavior.

And I would probably keep the VMOPTIONS appending to the main oieserver.vmoptions file, so if anyone provides the same option in both a mounted conf/custom.vmproperties and the env variable, the env variable will come later and replace the option from the file mount.

jonbartels
jonbartels previously approved these changes Jul 1, 2025
gibson9583
gibson9583 previously approved these changes Jul 1, 2025
kayyagari
kayyagari previously approved these changes Jul 2, 2025
@kpalang kpalang dismissed stale reviews from kayyagari and gibson9583 via 7159e52 July 2, 2025 08:13
@kpalang kpalang requested review from gibson9583 and kayyagari and removed request for gibson9583 July 2, 2025 08:43
kpalang and others added 4 commits July 2, 2025 12:10
Signed-off-by: Kaur Palang <kaur.palang@brightcodecompany.com>
Signed-off-by: Kaur Palang <kaur.palang@brightcodecompany.com>
Signed-off-by: Jon Bartels <jon.bartels@teladochealth.com>
Signed-off-by: Jon Bartels <jonathan.bartels@gmail.com>
Signed-off-by: Kaur Palang <kaur.palang@brightcodecompany.com>
@kpalang kpalang force-pushed the Issue-40-docker-image-for-releases branch from 753d5d2 to 23589e3 Compare July 2, 2025 09:13
@kpalang kpalang requested a review from gibson9583 July 2, 2025 09:16
@jonbartels jonbartels merged commit 8f656d4 into main Jul 2, 2025
1 check passed
@jonbartels jonbartels deleted the Issue-40-docker-image-for-releases branch July 2, 2025 18:17
Comment on lines +7 to +8
ARG UID=14285
ARG GID=14285
Copy link
Member

Choose a reason for hiding this comment

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

Expected behavior is that UID and GID are 1000 so that bind mount permissions work for the majority of users where their host UID is also 1000.

See nextgenhealthcare/connect-docker#49. This caused quite a few issues in the NextGen slack when the ubuntu image started including an ubuntu user with UID 1000 in their base images, so NextGen bumped the mirth UID to 1001.

Here is where they corrected it by renaming the ubuntu user to mirth so that the mirth user retained UID 1000. nextgenhealthcare/connect-docker@233502c

Copy link
Member

Choose a reason for hiding this comment

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

the other piece of that is even if you aren't using bind mounts, and you try to use existing volumes from a nextgen container, but the nextgen container had a different UID from the new oie container, the oie user won't have appropriate access to the existing volume.

@tonygermano tonygermano mentioned this pull request Jul 9, 2025
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.

8 participants