Switch to using Quinoa for UI backend, rather than Node.js + express

[holly-cummins]

Resolves #87.

Here's what I've done:

Code changes (the easy part!)

• Replaced node.js express app server with Quarkus app server
• Set up pom.xml for Quinoa
• Moved Angular app to location Quinoa expects
• Converted logic for serving environment variables from node.js to Java
• Removed express dependency from package.json

Testing

• Tested locally with quarkus dev
• Tested locally with mvn package and java -jar target/quarkus-app/quarkus-run.jar
• Tested locally with mvn package -Pnative and ./target/ui-super-heroes-1.0-runner … the front end starts in 27 ms on my machine, which is pretty awesome
• Tested locally with docker-compose -f deploy/docker-compose/app.yml up, although that gives me the old image which is still using node.js, so doesn’t tell me much

Deployment and automation

• Removed custom docker files and replaced them with standard Quarkus ones
• Added Quarkus Kubernetes extensions into the UI pom.xml
• Removed the special casing for UI projects in generate-k8s-resources
• Removed the special UI build steps from the GitHub workflows and added the ui project to the matrix builds. (The same matrix seems to be repeated three times, so I wonder if it should be common?)
• Updated app.openshift.io/runtime: nodejs to specify quarkus in all the Kubernetes .ymls, but I wasn’t sure how to test this

Documentation

• Updated readme (including architecture diagrams)
• Updated the azure instructions for running the UI app locally
• Updated automation documentation to remove special-casing of UI app. I hand-updated a diagram with a workflow screencap but we might want to regenerate it when this is merged and the workflows are running.

In both the k8s script and the automation docs we list out each project individually, to be able to exclude the UI project. That’s not needed anymore, so we may want to do some *-ing or “all” in the text, but I didn’t make those changes. We may also spot some other simplifications that come from having a homeogenous stack across all projects.

I’m not totally sure I have the instructions for Azure Static Web Apps right, but it’s hard to test. We’re still generating the same static app as we were before. Before it would ask a node.js server for its config, now it asks a Quarkus server for its config, so I’m not sure how either model exactly works with a static deployment.

[edeandrea]

Thank you @holly-cummins ! I'll take a closer look tomorrow or Friday, but first thing I noticed is that you shouldn't manually change anything in a folder named deploy, whether at the root of the project or inside one of the application folders. Everything in those folders are generated as part of the CI/CD process.

[holly-cummins]

Ah, thanks, @edeandrea. It seemed wrong to leave the wrong values in source control. :)

I assume CI/CD will just overwrite what's there with new content (which would be the same as what I generated), so my changes wouldn't do any harm?

[edeandrea]

I assume CI/CD will just overwrite what's there with new content (which would be the same as what I generated), so my changes wouldn't do any harm?

Yeah the CI process will essentially do an rm -rf **/deploy/**, regenerate everything, and then commit it back into source control.

Just for future reference - no need to ever touch anything inside any directory named deploy :)

I'll take a closer look at this tomorrow/friday and comment on it.

[holly-cummins]

Thanks, @edeandrea! I guess if we wanted to make it mega-obvious the files were generated we could put those folders in the .gitignore list and have CI/CD adjust the ignore and commit the deploy/*. That would be slightly more complicated scripting, though.

[edeandrea]

We could. I tried to make it obvious by having a README file in each directory saying not to manually modify anything in there as it'll be overwritten by the CI process.

Apparently I failed there too :)

[holly-cummins]

On another subject, I notice that the matrix of what's getting run is the one from main, not the one in this work item. That makes it harder to be confident my workflow changes are ok. https://github.com/holly-cummins/quarkus-super-heroes/actions/runs/5201812768 did run the changes from the workflow and is dying, but it's a permissions issue so I think it's something to do with the github token in that actions setup, not my file changes. But it's hard to know for sure. :(

[edeandrea]

Yeah you'd never want to run the build-push-container-images workflow on a fork because of that. It requires the token. Really only the simple-build-test (to verify the app is ok), but we don't have a good way of verifying that all the k8s yamls/etc all "look" right unfortunately.

[edeandrea]

I spent some time going through all of this and have made some comments in-line.

I also think that the docker compose stuff is missing. See src/main/docker-compose from some of the other apps as well as the scripts/generate-docker-compose-resources.sh. There probably need to be modifications in there as well.

Also you should check the scripts/deploy-to-azure-containerapps.sh script and verify nothing needs to change there.

[edeandrea]

Couple of things missing in here:

• Should define a quarkus.application.name.
• Should define a random test port. See

  quarkus-super-heroes/rest-villains/src/main/resources/application.properties

  Line 5 in 2f99db0

  quarkus.http.test-port=0

  for example.
• Probably should be some OpenTelemetry stuff in here. See

  quarkus-super-heroes/rest-villains/src/main/resources/application.properties

  Lines 26 to 29 in 2f99db0

  	# OpenTelemetry
  	quarkus.otel.resource.attributes=app=${quarkus.application.name},application=villains-service,system=quarkus-super-heroes
  	quarkus.otel.exporter.otlp.traces.endpoint=http://otel-collector:4317
  	quarkus.datasource.jdbc.telemetry=true

  for examples.
• Also missing the container image config. See https://github.com/quarkusio/quarkus-super-heroes/blob/2f99db07cfe69aa4313a9337fb2c76761d66850f/rest-villains/src/main/resources/application.properties#L47C1-L51 for examples.
• Should keep the logging config consistent with the rest of the apps. See https://github.com/quarkusio/quarkus-super-heroes/blob/2f99db07cfe69aa4313a9337fb2c76761d66850f/rest-villains/src/main/resources/application.properties#L15C9-L24 for examples.
• There is a lot of kubernetes/minikube/knative/openshift config here that is missing that should be here as well. See https://github.com/quarkusio/quarkus-super-heroes/blob/main/rest-villains/src/main/resources/application.properties#L53-L96 for examples.
• Also should define quarkus.knative.min-scale=1, quarkus.openshift.env.vars.CALCULATE_API_BASE_URL=true and quarkus.knative.env.vars.CALCULATE_API_BASE_URL=true

[holly-cummins]

Wow, that's a lot of config. It makes me think we're missing some defaults. But I've added it all in.

Do you know what the difference is between app= and application= in the otel attributes?

[edeandrea]

Its consistent with the labels we use in the kubernetes descriptors. In the case of this app, though, their value is probably the same because its a standalone app with nothing else. In the case of something like the villains app, app=rest-villains and application=villains-service.

As far as the rest of the config goes, we tried to introduce profiles to make it easier for users to deploy to various k8s variants without having to remember all the params they need to set. See https://github.com/quarkusio/quarkus-super-heroes/tree/main/rest-villains#deploying-directly-via-kubernetes-extensions for an example.

The README.md file in ui-super-heroes should probably also contain a lot of the same information as well now that there are multiple variants.

[holly-cummins]

If we did

%openshift.quarkus.config.profile.parent=kubernetes

instead of

%openshift.quarkus.config.profile.parent=prod

wouldn't we be able to drop a whole bunch of the copy-and-pasted config for the kubernetes-variants profiles?

And I feel like there should be a default for stuff like this:

quarkus.kubernetes.labels.app=${quarkus.application.name}

[edeandrea]

I'm not sure if the openshift extension will inherit things from the quarkus.kubernetes namespace, because there is the quarkus.openshift config namespace, and the documentation doesn't say anything about any kind of inheritance.

So, maybe, but untested.

[holly-cummins]

Ah, that's a good point about the namespace.

[edeandrea]

I spent some time going through all of this and have made some comments in-line.

I also think that the docker compose stuff is missing. See src/main/docker-compose from some of the other apps as well as the scripts/generate-docker-compose-resources.sh. There probably need to be modifications in there as well.

Also you should check the scripts/deploy-to-azure-containerapps.sh script and verify nothing needs to change there.

[edeandrea]

One other thing I noticed that I forgot to mention in the review - you'll need to modify the simple-build-test workflow and add the ui-super-heroes app to both of the matrix jobs.

[holly-cummins]

I spent some time going through all of this and have made some comments in-line.

I also think that the docker compose stuff is missing. See src/main/docker-compose from some of the other apps as well as the scripts/generate-docker-compose-resources.sh. There probably need to be modifications in there as well.

Done!

For another WI, do you think it's possible to reduce the volume of docker compose files? I was a bit perplexed by the relationship between rest-fights/deploy/docker-compose/native.yml and rest-fights/src/main/docker-compose/native.yml, for example. One seems to be generated and the other is hand-maintained? For the UI project they're basically identical, but for others there's overlap, but also different content. I couldn't figure out the intention from a search of the readmes. (And a readme probably isn't the right place for that kind of information, because people might not be able to find it mixed in with all the other content, and when they need is when they're looking at the file, trying to understand if it's ok to update or if it's generated.)

Also you should check the scripts/deploy-to-azure-containerapps.sh script and verify nothing needs to change there.

Good point, and done! I think because the externals of the UI container are exactly the same, no changes are needed.

[edeandrea]

For the docker compose stuff, yes src/main are hand maintained "snippets" whereas in the deploy directory is the "real" docker compose file. It's done this way so that we can have the root /deploy/docker-compose with an uber compose file that had everything from all the apps in it.

It was done that way to try and mimic the kubernetes extension. If we did away with that we'd have to hand craft 2 sets of compose files - 1 individually for each app and 1 uber compose file, both for jvm (and perhaps multiple jvm versions) and native.

[holly-cummins]

For the docker compose stuff, yes src/main are hand maintained "snippets" whereas in the deploy directory is the "real" docker compose file. It's done this way so that we can have the root /deploy/docker-compose with an uber compose file that had everything from all the apps in it.

It was done that way to try and mimic the kubernetes extension. If we did away with that we'd have to hand craft 2 sets of compose files - 1 individually for each app and 1 uber compose file, both for jvm (and perhaps multiple jvm versions) and native.

Got it, thanks. I've moved the discussion to #299.

[holly-cummins]

Ok, I think I've got all the comments. Thanks for spotting the issues, @edeandrea!

[edeandrea]

Assuming that means you think it's ready for another pass through? :)

Of so, I'll try to get to it later today

[holly-cummins]

Assuming that means you think it's ready for another pass through? :)

Of so, I'll try to get to it later today

Yes please, thank you!

[edeandrea]

@someth2say FYI once this is merged it's going to have an effect on #291. Should actually make the work you are doing there easier I would think, although you may get some merge conflicts. Please be aware.

[edeandrea]

We're close! See comments in-line.

[edeandrea]

Should be 3.1.1.Final to be consistent with the rest of the apps.

[holly-cummins]

Hmm, I can't comment on #297 (comment).

I don't think there's any point in getting too focussed on specific versions, is there? As soon as this merges, dependabot will take over and move to the latest version of dependencies. Until it merges, it's a manual process to try and keep adjusting the pom to reflect recent releases.

[edeandrea]

I don't think there's any point in getting too focussed on specific versions, is there

Just that the simple-build-test workflow runs against this PR. Not that I expect any failures, but the version update could potentially incorporate a problem which then would cause the main CI to break when this gets merged to main.

But you're right, probably not a big deal in this case.

[holly-cummins]

I don't think there's any point in getting too focussed on specific versions, is there

Just that the simple-build-test workflow runs against this PR. Not that I expect any failures, but the version update could potentially incorporate a problem which then would cause the main CI to break when this gets merged to main.

But you're right, probably not a big deal in this case.

I think if the version of Quarkus in one microservice being a week out of date caused that kind of problem, we would need to go back to the Quarkus team and have a chat about their release testing. :)

Although, actually, I don't think I entirely see the source of risk. If we think the simple-build-test workflow that we run against PRs isn't comprehensive enough to catch some problems, and we think the move from Quarkus 3.0.3 to 3.1.1 is a risk, wouldn't that risk still be there if I updated before merging? Because I'd be merging with Quarkus 3.1.1, but we wouldn't be confident of catching problems with that version? Or would we be assuming I'd be running more comprehensive tests locally with 3.1.1 than the PR workflow runs?

I think I'm missing something about the process and the concern.

[edeandrea]

I think I'm missing something about the process and the concern.

This particular app has very little (meaning close to 0) tests, whereas all the other apps have quite a big test suite, meaning the simple build test workflow is definitely trustworthy for all the other apps. In the year and a half I've been maintaining this, I've never had an issue where something was green in the PR and then things were broken once merged (in the other apps - I've had it happen with the UI app, which is why I've tried to not touch it with a 10-foot pole :) ). Hopefully #290 will help there.

[holly-cummins]

I think I'm missing something about the process and the concern.

This particular app has very little (meaning close to 0) tests, whereas all the other apps have quite a big test suite, meaning the simple build test workflow is definitely trustworthy for all the other apps. In the year and a half I've been maintaining this, I've never had an issue where something was green in the PR and then things were broken once merged (in the other apps - I've had it happen with the UI app, which is why I've tried to not touch it with a 10-foot pole :) ). Hopefully #290 will help there.

That makes sense. I'm just not convinced that there's much correlation between someone being a human-dependabot and updating a pom.xml before merging and them doing manual testing. :)

As you say, I think the solution is to improve the testing, not to try and do all version updates manually. Even if I updated to Quarkus 3.1.1 before merging, you know 3.1.2 would be along shortly, and then we have the exact same issue - "can we trust a version update if there's not someone to do manual testing of it before merging?"

[holly-cummins]

Ok, I’ve done the next round of changes, except for the following (as discussed above):

• Analyse all the readmes and scripts to identify files which are generated, and avoid changing them (Indicate generated yaml by .gitignore-ing it and including a comment indicating it's generated (to reduce maintenance effort for contributors) #299 should help with that in future)
• Monitor dependabot changes in the rest of the codebase and manually mirror them to this PR while the change is in review
• Restructure readme to include more instructions about how the quarkus kubernetes extension works

You’re quite right, @edeandrea, there should have been tests for the Java part. As there weren’t any for the old express server, I had kept parity, but we should aim higher.

I’ve also added the Quinoa incantation to run the Javascript tests, should we ever add any (#290). The javascript tests were so missing that npm run test wasn’t even a no op, it actually failed. So I’ve fixed that, although it ended up involving adding a skeleton test and loads of fixes to make the test framework work at all. The application didn’t even have any of the skeletons for testing, and finding an example of an angular test setup that could worked with Angular 12 was a bit arduous. (And, ultimately pointless, since we know we need to move away from Angular 12.) Retrofitting modern examples, of course, does not work. In the end, I got most of the way there but Jasmine insisted there were no test files. I spent about three hours debugging that but it really doesn’t seem like a good use of time, so in the end I just bypassed the actual Javascript test execution, but left the scaffolding I’d got and the Quinoa executor in.

I also spotted an incorrect package in the Java code, so I’ve fixed that.

[edeandrea]

Thank you very much @holly-cummins! I'll merge once CI has run.

[holly-cummins]

Thanks, @edeandrea!

[edeandrea]

There were a few things I missed too in my reviews that I realized when I checked out the branch. I just committed them myself to this PR.