Skip to content

Conversation

@Argelbargel
Copy link
Contributor

@Argelbargel Argelbargel commented May 22, 2025

Adds defaults to application.yaml of sophora-ugc:

  • adds reference to injected logback.xml
  • add defaults required by spring-security (without them the application terminates with an exception)
  • disable banner-mode (not useful when scraping logs)
  • explicitly declare the server port for the ugc-webapp (9080 is the port used when no configuration was given)

@Argelbargel Argelbargel force-pushed the add-application-defaults-to-values branch from f23833c to 5320ed9 Compare May 22, 2025 19:36
Copy link
Member

@philmtd philmtd left a comment

Choose a reason for hiding this comment

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

Hi @Argelbargel, did you verify that the logback configuration path does not work without this configuration?
The other settings are configuration options that I can see in the default configuration provided by the application and/or in the actual deployment's values.yaml file, but not in the Helm Chart's defaults. So I'd kindly ask you to remove these lines.

@Argelbargel
Copy link
Contributor Author

Hi @philmtd: the changes in this PR are the result of me trying to rollout this chart "as is" yesterday :-)

I'm sorry to say that they seem to be required. This and the other PRs are the direct translations of the kustomize-patches i had to run after rendering the chart to make it work.

The only change that i'm not sure of is disabling the banner-mode - this one i copied from our current vm-setup.

Setting server.port is important as the default setting in the chart without it is to expect the server to run on port 8080 and thus the ugc-webapp is neither reachable via the service nor the ingress (i've got another PR prepared which "fixes" the ingress, service and container-port-settings but want to include the ugc-multimedia, too)

Without setting spring.security the webapp terminates with an exception (reported in subshells jira yesterday as ZUPSWR-518).

And without explicitly setting the path to the logback.xml i did not get any further log-output beyond the exception.

@philmtd
Copy link
Member

philmtd commented May 23, 2025

I'm sorry to say that they seem to be required.

I understand that. But I still don't think this is the right place to put them. In my opinion and with the goal of an as clean as possible Helm chart, I see these values in the defaults provided by the application (work required by us) and/or in the installation values you use for your installation (either permanently or as a workaround until we put them into our defaults). There is no need to have these values in the default values.yaml of the chart.

The only option I think is ok here is the logback path configuration.

@Argelbargel
Copy link
Contributor Author

Argelbargel commented May 23, 2025

TBH: the logging-config should be set via an env-var IMO. That would be LOGGING_CONFIG= right?

Because neither the Docker-Image, nor the JAR it uses, require these exact default-values (the jar should use spring's default-locations and the image somthing like /config/application.yaml and /config/logback-spring.xml).

For the chart any other location than /config/logback/logback.xml does not make any sense, so this should not be changed (e.g. accidently be copy&pasting) by changing the values of the chart.

Concerning server.port - this should be either fixed somehow (does SERVER_PORT override any setting in application.yaml?) or it has to be in the values.yaml as this property is currently used to configure the containerPort (and the service-port, but that could - and should - be remidied by using port-names instead of numbers outside of the deployment spec, e.g. see https://github.com/Argelbargel/subshell-helm-charts/tree/fix-ugc-service-and-ingress).

But using a fixed value for the port used by the webapps-tomcat that can not be changed by the chart seems to be the best way to go as there is no need at all to make it changeable - especially as the chart does not allow to insert additional (init)Containers so there can not be any conflicts with other containers using the port.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants