Add support to build docker image with eclipselink #227

MonkeyCanCode · 2024-08-29T02:29:27Z

Description

As #114 added support for building the project with eclipselink via optional flag, this PR is add the same support via optional command line argument to run.sh when using mini-deployment locally.

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
Documentation update
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Default:

yong@DESKTOP:~/polaris(optional_eclipselink)$ bash run.sh
Building Kind Registry...
...
Creating cluster "kind" ...
...
Building polaris image with ECLIPSELINK=false...
...
Applying Kubernetes manifests...
namespace/polaris created
deployment.apps/polaris-deployment created
service/polaris-service created

With optional command line argument:

yong@DESKTOP:~/polaris(optional_eclipselink)$ bash run.sh -e true
Building Kind Registry...
...
Creating cluster "kind" ...
...
Building polaris image with ECLIPSELINK=true...
...
Applying Kubernetes manifests...
namespace/polaris created
deployment.apps/polaris-deployment created
service/polaris-service created

Helper:

xxx@DESKTOP:~/polaris(optional_eclipselink)$ bash run.sh -h
Usage: run.sh [-e true|false] [-h]
  -e    Set the ECLIPSELINK flag (default: false)
  -h    Display this help message

Checklist:

Please delete options that are not relevant.

I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
If adding new functionality, I have discussed my implementation with the community using the linked GitHub issue

run.sh

eric-maynard · 2024-08-29T17:11:58Z

run.sh

+usage() {
+  echo "Usage: $0 [-e true|false] [-h]"
+  echo "  -e    Set the ECLIPSELINK flag (default: false)"
+  echo "  -h    Display this help message"
+  exit 1
+}


This change is one I'm a little unsure about. Things like metastore manager and callcontext resolver are designed to be pluggable and discoverable by DropWizard.

I'm worried about the precedent this sets and whether we would want to add all these as myriad options here. The script might get quite complicated in order to cover every combination.

With this concern, should we move those into a config file instead?

Or maybe just a safety valve type flag that allows you to pass an arbitrary build arg down to docker build?

K, let me think about it tonight then submit another PR to address those tonight.

@eric-maynard updated with arbitrary build arg support. Please take another look when you get a chance.

Helper message:

Usage: run.sh [-b build-arg1=value1,build-arg2=value2,...] [-h] -b Pass a set of arbitrary build arguments to docker build, separated by commas -h Display this help message

Default run:

bash run.sh

Arbitrary build arg run:

bash run.sh -b ECLIPSELINK=true # sample output: # Building polaris image with build arguments: --build-arg ECLIPSELINK=true

Arbitrary build args run:

bash run.sh -b ECLIPSELINK=true,RANDOM=true # sample output: # Building polaris image with build arguments: --build-arg ECLIPSELINK=true --build-arg RANDOM=true

It would be nice to add a change to the docs specifying how to get this build working. But I don't want to hold the PR up any longer, so maybe post a separate docs PR

I can update those later tonight if not yet merged.

actually lets have it in a diff PR. The current Dockerfile doesn't support eclipseLinkDeps flag. I will add this in and update the doc for both optional options. What do u think?

@flyrain

I'd recommend to add an example like ./run.sh -b ECLIPSELINK=true, as it is probably most commonly used.

eric-maynard · 2024-09-04T19:03:02Z

run.sh

-# build and deploy the server image
+# Check if BUILD_ARGS is not empty and print the build arguments
+if [[ -n "$BUILD_ARGS" ]]; then
+  echo "Building polaris image with build arguments:$BUILD_ARGS"


nit: space missing after :

eric-maynard · 2024-09-04T19:03:21Z

run.sh

 echo "Applying kubernetes manifests..."
 kubectl delete -f k8/deployment.yaml --ignore-not-found
-kubectl apply -f k8/deployment.yaml
+kubectl apply -f k8/deployment.yaml


Missing whitespace at end of file

eric-maynard

LGTM with some comments on style

MonkeyCanCode · 2024-09-10T18:30:09Z

Anything else needed on this PR?

RussellSpitzer · 2024-09-10T19:07:35Z

The license check is currently failing, I don't think this should be happening, can you try rebasing and try again?

MonkeyCanCode · 2024-09-10T19:32:34Z

The license check is currently failing, I don't think this should be happening, can you try rebasing and try again?

yes. Just noticed it. Was fine earlier. Had being a while since I submitted this PR. The PR had being rebased.

adutra · 2024-09-11T08:51:48Z

The license check is currently failing, I don't think this should be happening, can you try rebasing and try again?

yes. Just noticed it. Was fine earlier. Had being a while since I submitted this PR. The PR had being rebased.

Probably fixed by #266 or #271.

flyrain

+1

flyrain · 2024-09-21T06:05:21Z

Thanks @MonkeyCanCode for the PR. Thanks @RussellSpitzer @eric-maynard @collado-mike for the review.

Add support to build docker image with eclipselink

b7784e2

MonkeyCanCode requested review from RussellSpitzer, ashvina, jackye1995, jbonofre, snazy, takidau and vvcephei as code owners August 29, 2024 02:29

eric-maynard reviewed Aug 29, 2024

View reviewed changes

run.sh Outdated Show resolved Hide resolved

eric-maynard reviewed Aug 29, 2024

View reviewed changes

MonkeyCanCode added 2 commits August 29, 2024 14:25

add suppor for arbitrary args

f5c6b51

Fix conflict

ab7f52a

eric-maynard reviewed Sep 4, 2024

View reviewed changes

eric-maynard approved these changes Sep 4, 2024

View reviewed changes

Fix nit

162f53e

RussellSpitzer approved these changes Sep 10, 2024

View reviewed changes

Merge branch 'main' into optional_eclipselink

36486ea

eric-maynard mentioned this pull request Sep 11, 2024

[BUG] docker build fails when ECLIPSELINK arg is set to true #280

Closed

1 task

flyrain approved these changes Sep 21, 2024

View reviewed changes

flyrain merged commit a0b24ce into apache:main Sep 21, 2024

This was referenced Sep 23, 2024

Add eclipseLinkDeps options for Dockerfile and run.sh #313

Merged

Dockerfile: Add build arg for eclipselink dependencies #311

Merged

MonkeyCanCode deleted the optional_eclipselink branch December 12, 2024 04:58

Add support to build docker image with eclipselink #227

Add support to build docker image with eclipselink #227

Uh oh!

Conversation

MonkeyCanCode commented Aug 29, 2024

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

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!

MonkeyCanCode Aug 29, 2024 • 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!

eric-maynard left a comment

Choose a reason for hiding this comment

Uh oh!

MonkeyCanCode commented Sep 10, 2024

Uh oh!

RussellSpitzer commented Sep 10, 2024

Uh oh!

MonkeyCanCode commented Sep 10, 2024

Uh oh!

adutra commented Sep 11, 2024

Uh oh!

flyrain left a comment

Choose a reason for hiding this comment

Uh oh!

flyrain commented Sep 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

MonkeyCanCode Aug 29, 2024 •

edited

Loading