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.

Sign up for GitHub

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

Kube-Series-2.1: Features/properties-makefile-dockerfile #199

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Kube-Series-2.1: Features/properties-makefile-dockerfile #199

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1d0712d
Refactored application properties system
Dec 20, 2019
c9694fc
Fixed start.sh as discussed in the PR
Jan 4, 2020
a43abe8
Fix start script big mistake
bygui86 Jan 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions buildAndRun.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
#!/bin/bash
#!/bin/sh

## These are the required ENV variables for startup for development
## Define what configs to load on start
export SPRING_CONFIG_LOCATION=classpath:/config/base.yml,classpath:/config/dev.yml
# Define a profile to load specific configurations
# Available profiles: [null = default = prod] | dev
export SPRING_PROFILES_ACTIVE="dev"

## What port to listen on
export PORT=8080
# Define which project should be run (comma separated list)
export PROJ_LIST="kafka-webview-ui"

## run application via maven
mvn -pl kafka-webview-ui spring-boot:run -DskipCheckStyle=true -DskipTests=true -DskipLicenseCheck=true
# Define maven configurations
export MAVEN_CONFIG="-DskipCheckStyle=true -DskipTests=true -DskipLicenseCheck=true"

# Run application using maven
mvn spring-boot:run -pl $PROJ_LIST $MAVEN_CONFIG
3 changes: 0 additions & 3 deletions kafka-webview-ui/src/assembly/distribution/start.bat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@

cd /D %~dp0

:: Define configuration
set SPRING_CONFIG_LOCATION=classpath:/config/base.yml,config.yml

:: launch webapp
FOR /F "tokens=* USEBACKQ" %%F IN (`dir /b kafka-webview-ui-*.jar`) DO SET "jar=%%F"
java -jar %jar%
48 changes: 30 additions & 18 deletions kafka-webview-ui/src/assembly/distribution/start.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,35 @@
#!/bin/bash
#!/bin/sh

CWD=`pwd`
# Get the directory of this script
SCRIPT_DIR=$(dirname "$0")

## Change to local directory
cd "${0%/*}"

# Define empty options as defaults if none set
Copy link
Collaborator

Choose a reason for hiding this comment

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

changing these environment names will break backwards compatibility yea?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested it manually and I don't see problems.
Unfortunately I was not able to test in Docker, because the build fail. It's not able to download from the artifact repo :(
As soon as I can, I will attach logs for this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the issue is if someone has built a deployment process around using these environment variables, removing/renaming them would be a breaking change for them as they would need to update their deployment to use the new variable names.

I'd prefer to maintain backwards compatibility where possible to avoid causing version upgrade pains/difficulties for people.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know what you mean, but sometimes it's unavoidable to have some kind of disruptions after a while between two different versions. The main example is Java 8 to 9. Some disruptive changes were unavoidable and better for the future of Java in general.
Moreover if the documentation is written well, I think everyone can figure out easily what to change and how.

Analysing specifically these changes:

  • HEAP_OPTS refers just to some specific Java configurations about the memory, what about other configurations? Like -noverify -server -XX:TieredStopAtLevel=1 these should not be included as this would mean misusing this specific environment variable. A variable called JVM_OPTS and JAVA_OPTS are more general purpose and usable in many different flexible ways.
  • LOG_OPTS can be restored back, but it's kind of the same as HEAP_OPTS.
  • The usage of SPRING_CONFIG_LOCATION is completely the same, I removed this as I already included everything in the two default files /resources/application.yaml and /resources/application-dev.yaml. This has 100% backward compatibility.

Moreover the definition of empty environment variables like

if [[ -z "$HEAP_OPTS" ]]; then
    export HEAP_OPTS=""
fi
if [[ -z "$LOG_OPTS" ]]; then
    export LOG_OPTS=""
fi

is useless and equivalent to not defining them, so they can be removed without backward compatibility problems anyway.

I missed to share with you a bigger vision on my series of PRs. In the last one, improving Dockerfile, I would remove the usage of start.sh in the Dockerfile, in favour to something like following:

FROM openjdk:8-jre-alpine

...

ENTRYPOINT exec java $JVM_OPTS $LOG_OPTS $JAVA_OPTS -jar kafka-webview-ui-*.jar

start.sh does nothing special/specific, so this could be avoided in favour to an easier Dockerfile and no startup scripts, replacing some useless complexity.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In regards to the start.sh changes. It may be better to have a check if the to be deprecated environment variables are used, and display an appropriate warning and migration information. Until removal, it should be easy to merge the previous variable into the new one.

I see no issues with the Dockerfile avoiding the start.sh entirely as you've laid above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it.
Do you think that the Makefile (to be introduced by PR #200 ) is enough as alternative to start.sh?

Probably doesn't make sense to package a Makefile with a binary distribution that's already compiled.

RE: Docker failing to find the archive from sonatype... I'm not sure. It looks like all of the releases were removed from their maven repository, I'll need to look into why.

You should be able to swap out the URL for the copy hosted on github here: https://github.com/SourceLabOrg/kafka-webview/releases/download/v2.5.0/kafka-webview-ui-2.5.0-bin.zip

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably doesn't make sense to package a Makefile with a binary distribution that's already compiled.

Ah ok, you mean really deploy just the jar together with the script.sh to be deployed on prem. Ok got it. I will think of something like that ;)

You should be able to swap out the URL for the copy hosted on github here: https://github.com/SourceLabOrg/kafka-webview/releases/download/v2.5.0/kafka-webview-ui-2.5.0-bin.zip

I will prepare some different Dockerfile to have more options to build the project. For example a Dockerfile.local to take the jar from local, in order to be able to use a very custom version of Kafka-WebView and a Dockerfile.release to be used for an official release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @Crim, sorry to let you wait, Xmas time here around.

I made the changes we discussed on the start.sh, please have a look at it!
After my changes in the last PR of this series, I would like to discuss how to deal with the start scripts. I don't understand why they are inside src/assembly for example.

I will put all changes to the Dockerfile in the last PR of this series.

Copy link
Collaborator

@Crim Crim Jan 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

So after reviewing this file, I'm not sure it works like you might expect it to. I've modified line 25 below to show what command would be run.

# Start application
echo java $JVM_OPTS $MEM_OPTS $JAVA_OPTS -jar kafka-webview-ui-*.jar

As written its impossible to set the new environment variables and have them be used. Example:

SMBP:tmp $ export JAVA_OPTS="-Xms1G -Xmx5G"
SMBP:tmp $ ./test.sh 
java -noverify -server -XX:TieredStopAtLevel=1 -Xms2G -Xmx2G -XX:MaxMetaspaceSize=300M -jar kafka-webview-ui-*.jar

I believe you'd need the if [[ -z .... checks to default the the values, instead of just hardcoding the environment variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Crim I'm really sorry! I made a big mistake.
I just pushed a new version of the script. I tested it more extensively with zsh and bash, it works as expected now. Please test it and let me know.

Big changes:

  • removed the folder change, it's not necessary if we take care of find the directory of the script and put it in front of the jar file name
  • removed exec from java execution, not necessary as java command is blocker anyway

if [[ -z "$HEAP_OPTS" ]]; then
export HEAP_OPTS=""
# Define environment variables
## New
if [[ -z "$JVM_OPTS" ]]; then
export JVM_OPTS="-noverify -server -XX:TieredStopAtLevel=1"
fi
if [[ -z "$LOG_OPTS" ]]; then
export LOG_OPTS=""
if [[ -z "$MEM_OPTS" ]]; then
export MEM_OPTS="-Xms2G -Xmx2G -XX:MaxMetaspaceSize=300M"
fi
if [[ -z "$JAVA_OPTS" ]]; then
export JAVA_OPTS=""
fi
## Deprecated
if [[ ! -z "$LOG_OPTS" ]]; then
echo "Usage of 'LOG_OPTS' is deprecated and will be removed in the future, please switch to 'JAVA_OPTS'"
export JAVA_OPTS="$JAVA_OPTS $LOG_OPTS"
fi
if [[ ! -z "$HEAP_OPTS" ]]; then
echo "Usage of 'HEAP_OPTS' is deprecated and will be removed in the future, please switch to 'MEM_OPTS'"
export MEM_OPTS="$MEM_OPTS $HEAP_OPTS"
fi

## Define configuration
export SPRING_CONFIG_LOCATION=classpath:/config/base.yml,config.yml

## launch webapp
exec java -jar kafka-webview-ui-*.jar $HEAP_OPTS $LOG_OPTS

## Change back to previous directory
cd $CWD
# Start application
echo "Start Kafka-WebView with following parameters:"
echo "\tJVM_OPTS: $JVM_OPTS"
echo "\tMEM_OPTS: $MEM_OPTS"
echo "\tJAVA_OPTS: $JAVA_OPTS"
echo "\tLOG_OPTS: deprecated, added to JAVA_OPTS"
echo "\tHEAP_OPTS: deprecated, added to MEM_OPTS"
echo
java $JVM_OPTS $MEM_OPTS $JAVA_OPTS -jar kafka-webview-ui-*.jar
13 changes: 10 additions & 3 deletions ...view-ui/src/main/resources/config/dev.yml → ...ui/src/main/resources/application-dev.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,27 @@

# General
server:

servlet:
session:
persistent: true


# Spring
spring:

jpa:
show-sql: true
# H2 Datbase

h2:
console:
## Disable for release
enabled: true
path: /h2


# Kafka WebView custom
app:
requireSsl: false

user:
enabled: true
## Development server Ldap auth settings
Expand Down
88 changes: 85 additions & 3 deletions ...w-ui/src/assembly/distribution/config.yml → ...iew-ui/src/main/resources/application.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,95 @@

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it not make sense to have this use a profile as well to avoid having to redefine every property in this config?

Instead I'd like to be able to only surface the most relevant/common settings and hide away the complexities of configuring springboot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it not make sense to have this use a profile as well to avoid having to redefine every property in this config?

Spring Profiles are intended to make easier the customisation (add, update) of specific application properties. We don't have to redefine every time all properties in each configurations. The example is application-dev.yaml:

  • server.servlet.session.persistent is set to true instead of the default false
  • spring.jpa.show-sql = true is added
    Or did I understand wrong your question?

Instead I'd like to be able to only surface the most relevant/common settings and hide away the complexities of configuring springboot.

We can hide Spring Boot complexity defining some easier environment variables. But I don't think that changing completely the way configurations are set is an easier way, taking away some complexity. Instead you have to figure out a new way, more difficult and not really well documented compared to the standard Spring Boot way.
Instead we can think about something like this:

spring:
  application:
    name: ${APP_NAME:"Kafka Webview"}
management:
  server:
    port: ${MANAGEMENT_PORT:9090}

Already done by you before for the server.port:

server:
  port: ${PORT:8080}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way I described above about defining easier environment variables, we can decide which configuration should be easy to be overridden and provide documentation for that. If someone wants to override something else deeper in Spring Boot, it's his/her duty to understand how.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So what do you think about my last comment?
Can we define a set of easy-to-configure properties like suggested above and the rest will remain up to the user?

# General
server:

## What port to run the service on.
port: 8080

tomcat:
remote_ip_header: x-forwarded-for
protocol_header: x-forwarded-proto
mbeanregistry:
enabled: true

servlet:
session:
## User sessions should not be persistent across service restarts by default.
persistent: false
## User login session timeout after 1 hour (3600 seconds)
timeout: 3600


## Various App Configs
# Spring
spring:

application:
name: Kafka Webview

datasource:
url: "jdbc:h2:file:./data/db"
username: sa
password:
driver-class-name: org.h2.Driver

flyway:
locations: "classpath:schema/migration/{vendor}"
baselineOnMigrate: true

servlet:
multipart:
max-file-size: 64MB
max-request-size: 64MB

thymeleaf:
cache: false
enabled: true
prefix: "classpath:/templates/"
suffix: ".html"
mode: HTML
encoding: UTF-8
check-template-location: true

jpa:
hibernate:
ddl-auto: none
show-sql: false
open-in-view: true


# Spring Actuator - Global
management:
server:
port: 9090
endpoints:
web:
exposure:
include: "*"
endpoint:
health:
show-details: when_authorized
metrics:
tags:
application: ${spring.application.name}
## Disable LDAP by default
health:
ldap:
enabled: false

# Spring Actuator - Info endpoint
info:
app:
name: ${spring.application.name}


# Kafka WebView custom
app:

## Application name
name: Kafka WebView

## TODO: add a description to this configuration
uploadPath: "./data/uploads"

## Should be unique to your installation.
## This key will be used for symmetric encryption of JKS/TrustStore secrets if you configure any SSL enabled Kafka clusters.
key: "SuperSecretKey"
Expand Down Expand Up @@ -36,13 +117,14 @@ app:
## Setting to false will disable login requirement.
enabled: true

## Ldap auth settings
## Optional: if you want to use LDAP for user authentication instead of locally defined users.
ldap:
## Disabled by default.
enabled: false

## URL/Hostname for your LDAP server
url: "ldap://localhost:8389/dc=example,dc=org"
url: "ldap://localhost:63182/dc=example,dc=org"

## Example values defined below, adjust as needed.
## How to find user records
Expand Down Expand Up @@ -73,4 +155,4 @@ app:
## If LDAP does not allow anonymous access, define the user/password to connect using.
## If not required, leave both fields empty
bindUser: "cn=ManagementUser"
bindUserPassword: "password-here"
bindUserPassword: "password-here"
100 changes: 0 additions & 100 deletions kafka-webview-ui/src/main/resources/config/base.yml
View file
Open in desktop

This file was deleted.