CALC-58: Added support for Bamboo Specs. by Ruhanga · Pull Request #14 · openmrs/openmrs-module-calculation

Ruhanga · 2025-06-09T09:29:26Z

This PR adds Bamboo Specs involving JDK 21 and 24 since those are the only ones have successful builds.

https://openmrs.atlassian.net/browse/CALC-58

…n tasks

Ruhanga · 2025-06-16T12:21:01Z

Hi @dkayiwa, could we merge this?

dkayiwa · 2025-06-16T12:30:03Z

What failures do you get for the other Java versions?

Ruhanga · 2025-06-16T12:43:06Z

This master branch was updated a few days ago. I missed this. Let me update this PR accordingly.

Ruhanga · 2025-07-16T12:35:05Z

Hi @dkayiwa, I think this might have slipped through the cracks. Could we go ahead and get it merged?

dkayiwa · 2025-07-16T12:38:24Z

bamboo-specs/bamboo.yml

+
+        export IMAGE="maven:3.9.9-amazoncorretto-8"
+
+        docker pull ${IMAGE}


By the way, do we need to explicitly pull? Shouldn't run automatically do that?

By the way, do we need to explicitly pull?

No, just for explicitly's sake. It's not harmful but I could remove it.

I like reviewing a smaller number of lines :)

Should be a lighter review now with the latest commit... :-)

dkayiwa · 2025-07-16T13:37:03Z

bamboo-specs/bamboo.yml

+      scripts:
+      - |-
+        #!/bin/bash -eu
+


Are these two lines in between?

I see one line in between, line 57.

dkayiwa · 2025-07-16T13:42:53Z

bamboo-specs/bamboo.yml

+
+        set -x
+
+        export IMAGE="maven:3.9.9-amazoncorretto-8"


Do we still need the above line?

Yep, it’s just to make it easier for others to find the right variable when making changes.

I am not sold to variables for one time usages. I usually defer to the times when such need actually arises. I am a fun of reviewing and maintaining less code 😊

I guess, it comes down to balancing between less code and clarity, especially when it involves variables that might change later. Personally, I’d prefer more explicit and readable lines over fewer, less clear ones. I'm happy to change in favour of fewer lines though as I think this may not be the best example.

When you say might change, many times this never happens. That is how we end up with lots of things in our code bases that are never used. But a developer thought that this might be of use in the future. I do not see us reducing the clarity of this code by using maven:3.9.9-amazoncorretto-8, unless we want to use it to teach others the meaning of the parameters for the docker run command. 😊

I like this... Thanks for clarifying @dkayiwa.

dkayiwa · 2025-07-16T14:18:37Z

bamboo-specs/bamboo.yml

+
+        export IMAGE=${bamboo.build.docker.image.id}
+
+        docker pull ${IMAGE}


Do we need to pull here?

Yes, to ensure we’re using the latest of the provided image if not pulled yet.

Wouldn't the docker run automatically do that?

At-times no especially if the image available in the local cache exists even when not the latest.

Aren't we using a specific image version?

The one provided at that build stage varies and so is not specific. It does have a default though.

If you provided this to the docker run command ${bamboo.build.docker.image.id}, will it run a different one from ${IMAGE} ?

So what is the added value?

Should now be amended below.

dkayiwa · 2025-07-16T16:49:09Z

bamboo-specs/bamboo.yml

+
+        set -x
+
+        docker pull ${bamboo.build.docker.image.id}


Do we need to pull?

Yes, for the reason I had share in the previous review. Basically to ensure that the image version passed in is always the latest, otherwise cached older variants could be used.

Doesn't ${bamboo.build.docker.image.id} point to a specific id?

Yes, but there's provision to override that version which is the reason for ensuring safe override.

Do you mean that if you run docker, without an explicit pull, it will not pull the exact version contained in ${bamboo.build.docker.image.id}?

This depends on whether the image was cached. Cached images can become stale, and Docker won’t always pull the latest version if the image already exists on the machine.

Isn't that the same behaviour for the docker pull command? Will docker pull again an image which already exists?

No. With docker run if a local cache exists, docker won't bother pulling an updated version which is why there's need for explicitly execute docker pull.

dkayiwa · 2025-07-16T16:49:30Z

bamboo-specs/bamboo.yml

+
+        set -x
+
+        docker pull ${bamboo.build.docker.image.id}


Do we need to pull?

Yes, for the reason I had share in the previous review. Basically to ensure that the image version passed in is always the latest, otherwise cached older variants could be used.

Docker images are immutable. So if you are using a specific version ${bamboo.build.docker.image.id}, how can it be modified and stay as the same version.

Let's consider using maven:3.9.9-amazoncorretto-21. It could be any compatible variant, as that version may come in different flavors depending on the specific minor Java 21 updates included (e.g. 21.0.1, ..., 21.0.8 e.t.c). So if an older variant cache is present, the newer one is not pulled automatically by docker run.

Can you point me to the documentation where you are reading this from?

This is one of the docs...

... if a new version of the image being used is available, it doesn't get downloaded before the jobs run. This can cause jobs to fail if new features have been added to the image and are required for the new workflow to succeed.

Cause
Bamboo uses a cached image on the build agent. If Docker finds the tag locally, it won't check the remote registry for an updated version of the same tag.

Can i look at some of the values pointed at by {bamboo.build.docker.image.id} for the previous runs?

An example is maven:3.9.9-amazoncorretto-21, the default as seen on line 191;

It could be any other builder image tag. (Some other possible values)

Ruhanga requested review from dkayiwa, rkorytkowski and wikumChamith June 9, 2025 09:29

CALC-58: Added support for Bamboo Specs.

1647670

Ruhanga force-pushed the CALC-58 branch from 9889203 to 1647670 Compare June 9, 2025 09:47

CALC-58: Disable forced clean builds and remove permissions resolutio…

e1cf479

…n tasks

CALC-58: Added support for Java 8, 11 and 17

9711e7e

dkayiwa reviewed Jul 16, 2025

View reviewed changes

CALC-58: Remove unnecessary docker pull command executions.

9bce821

dkayiwa reviewed Jul 16, 2025

View reviewed changes

CALC-58: Cleanup unnecessary lines.

da76a10

dkayiwa reviewed Jul 16, 2025

View reviewed changes

CALC-58: Remove unnecessary IMAGE variable usage.

7d5136f

dkayiwa reviewed Jul 16, 2025

View reviewed changes

dkayiwa merged commit d5e64b5 into openmrs:master Jul 21, 2025
5 checks passed


		export IMAGE="maven:3.9.9-amazoncorretto-8"

		docker pull ${IMAGE}


		export IMAGE=${bamboo.build.docker.image.id}

		docker pull ${IMAGE}

Conversation

Ruhanga commented Jun 9, 2025

Uh oh!

Ruhanga commented Jun 16, 2025

Uh oh!

dkayiwa commented Jun 16, 2025

Uh oh!

Ruhanga commented Jun 16, 2025

Uh oh!

Ruhanga commented Jul 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ruhanga Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkayiwa Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkayiwa Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Ruhanga Jul 16, 2025 •

edited

Loading

dkayiwa Jul 16, 2025 •

edited

Loading

dkayiwa Jul 16, 2025 •

edited

Loading

Ruhanga Jul 17, 2025 •

edited

Loading