-
Notifications
You must be signed in to change notification settings - Fork 681
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
GUACAMOLE-682: docker build with optional maven profile #347
Conversation
To include library for RADIUS authentication in the docker image the build needs to activate the maven profile "lgpl-extentions" and copy the library into the image. The docker start script needs to pass through settings and link the library to GUACAMOLE_HOME.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution - this looks pretty good to me, though I'd like for someone from the project a little more familiar with Docker builds to weigh in. I have a couple of style nit-picks in one of the files, but overall looks really good.
mvn package | ||
else | ||
mvn -P "$BUILD_PROFILE" package | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please stick with accepted style of four-space indentations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
corrected
|
||
if [ -f extensions/guacamole-auth-radius/target/guacamole-auth-radius*.jar ]; then | ||
mkdir -p "$DESTINATION/radius" | ||
cp extensions/guacamole-auth-radius/target/guacamole-auth-radius*.jar "$DESTINATION/radius" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with above, four-space indentations, please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
corrected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good go to me. @mike-jumper you okay with these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Just missing some documentation.
@@ -41,6 +41,7 @@ | |||
|
|||
BUILD_DIR="$1" | |||
DESTINATION="$2" | |||
BUILD_PROFILE="$3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new BUILD_PROFILE
parameter for build-guacamole.sh
needs to be documented with an @param
in the corresponding comment block:
guacamole-client/guacamole-docker/bin/build-guacamole.sh
Lines 21 to 40 in 516dbfd
## | |
## @fn build-guacamole.sh | |
## | |
## Builds Guacamole, saving "guacamole.war" and all applicable extension .jars | |
## using the guacamole-client source contained within the given directory. | |
## Extension files will be grouped by their associated type, with all MySQL | |
## files being placed within the "mysql/" subdirectory of the destination, all | |
## PostgreSQL files being placed within the "postgresql/" subdirectory of the | |
## destination, etc. | |
## | |
## @param BUILD_DIR | |
## The directory which currently contains the guacamole-client source and | |
## in which the build should be performed. | |
## | |
## @param DESTINATION | |
## The directory to save guacamole.war within, along with all extension | |
## .jars. Note that this script will create extension-specific | |
## subdirectories within this directory, and files will thus be grouped by | |
## extension type. | |
## |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added the parameter documentation as requested
Extending docker build process to allow passing maven profile to include radius auth.