-
Notifications
You must be signed in to change notification settings - Fork 49
Conversation
Signed-off-by: Tim Schlagenhaufer <tim.schlagenhaufer@bosch.io>
Signed-off-by: Tim Schlagenhaufer <tim.schlagenhaufer@bosch.io>
…et + typings Signed-off-by: Tim Schlagenhaufer <tim.schlagenhaufer@bosch.io>
Signed-off-by: Tim Schlagenhaufer <tim.schlagenhaufer@bosch.io>
Signed-off-by: Tim Schlagenhaufer <tim.schlagenhaufer@bosch.io>
EXPOSE 8080 | ||
CMD java -XX:+UnlockExperimentalVMOptions -Dcom.sun.management.jmxremote ${JAVA_OPTS} -jar business-partner-agent.jar | ||
CMD ./setup-runtime.sh public/env.js business-partner-agent.jar && java -XX:+UnlockExperimentalVMOptions -Dcom.sun.management.jmxremote ${JAVA_OPTS} -jar business-partner-agent.jar |
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.
How does this impact start performance?
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.
About 5 seconds, the script will start after everything else has been set up
COPY --from=VUE /frontend/setup-runtime.sh setup-runtime.sh | ||
|
||
RUN \ | ||
mkdir public && \ |
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.
Why the public folder, are the contents of the .jar extracted there?
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.
The public folder is necessary because the jar tool ("jar uf") requires the same path of the file to modify the file in the existing jar file. See: https://docs.oracle.com/javase/tutorial/deployment/jar/update.html
There does not seem to be way around this (the jar uf -C option does not provide an alternative)
@@ -6,26 +6,26 @@ | |||
"serve": "vue-cli-service serve", | |||
"build": "vue-cli-service build", | |||
"test:unit": "vue-cli-service test:unit", | |||
"lint": "vue-cli-service lint && npm run license-file-headers-add", |
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.
where is this now?
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.
The license-file-headers-add script is called via lint-staged, so in .lintstagedrc.json.
It should not be included in the build, as it could worst case create pending file changes, which would probably impact build pipelines.
Headers should be created during development, before committing.
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.
makes sense. the backend build fails in this case is it possible to have the same behavior here?
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.
Yes, the license-check-and-add package offers a "check" option we could use in the build script (before starting the actual "vue-cli-service build" task). If it fails, the build fails. I can add it in a new commit
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.
Done
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.
Can you have a look to the build.yml there we have:
# Test Phase
- name: Test with npm
## TODO are there any frontend tests?
run: npm --prefix frontend run license-check
I think it makes sense to add something similar
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.
Yes, we can add one here too. The build should fail as early as possible in this case (missing license headers) as it is not resource intensive. Where do you think we should add it in the build.yml file?
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 another step in the test phase
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 step in new commit
Signed-off-by: Tim Schlagenhaufer <tim.schlagenhaufer@bosch.io>
Signed-off-by: Tim Schlagenhaufer <tim.schlagenhaufer@bosch.io>
"lint": "vue-cli-service lint && npm run license-file-headers-add", | ||
"prettify": "prettier --write **/*.{css,html,js,json,mjs,scss,ts,tsx,yml,md,vue}", | ||
"lint": "vue-cli-service lint", | ||
"prettify": "npm run license && prettier --write **/*.{css,html,js,json,mjs,scss,ts,tsx,yml,md,vue}", |
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.
npm run license is also called in the build.yml will this cause issues?
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 most likely won't cause any issues as it only creates a new licenses/licenseInfos.json file. Since it won't be committed in the build, it does not cause any problems. It actually is a good thing it's called in the build, since the result will always have the current dependency versions listed in the frontend.
The prettify script is currently for development and not part of a build chain (prettier calls should always happen before pushes and not on the pipeline).
# Second argument: Path to BPA jar file to replace env.js | ||
echo "Start setting runtime variables" | ||
|
||
# Overwrite frontend runtime variables with environment variables from container |
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.
What happens if the properties are not set, what is the default? Also would it make sense to skip in this case, meaning wrap this into something like if [ -z ... ];
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.
When the properties/env variables are not set, the system will pass an empty string ("") which replaces the value of the respective identically named environment variable in env.js. Example:
MY_VARIABLE is not set in e.g. docker-compose
which then turns this
window.env = { MY_VARIABLE: "__MY_VARIABLE__", };
into this:
window.env = { MY_VARIABLE: "", };
inside the jar.
If MY_VARIABLE is set to e.g. "true", "false" or anything else (e.g. "asdasd") it will be replaced accordingly.
Naturally, validation of the value needs to be done in the frontend, which for boolean values is included in this PR.
An empty string or an incorrect value does not break the code, it will be turned into "undefined".
Skipping this would make sense to save a little bit of startup time. We probably should discuss these kind of changes beforehand or in a different draft.
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 this is something only some users need we should keep the impact on the others as minimal as possible so I prefer the option to skip. We can do this in a second pr if you like.
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.
Yes, I totally agree. Since this functionality is now stable and testing takes a lot of time (rebuilding Docker images on changes) we should create a separate PR and branch to improve this script.
A.k.a if no variable is set - skip all. Only perform the edit for variables that are set.
Signed-off-by: Tim Schlagenhaufer <tim.schlagenhaufer@bosch.io>
frontend/package.json
Outdated
@@ -4,28 +4,29 @@ | |||
"private": true, | |||
"scripts": { | |||
"serve": "vue-cli-service serve", | |||
"build": "vue-cli-service build", | |||
"build": "npm run license-file-headers-check && vue-cli-service build", |
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 this is now a build step this should not be needed anymore
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.
Removed in most recent commit
…from build script Signed-off-by: Tim Schlagenhaufer <tim.schlagenhaufer@bosch.io>
To hide the sidebar entirely, set both to true e.g. in your docker-compose as environment variables