plantuml / plantuml-server

PlantUML Online Server
https://plantuml.com/
GNU General Public License v3.0
1.65k stars 472 forks source link

Update jetty and tomcat to latest version #198

Closed HeinrichAD closed 2 years ago

HeinrichAD commented 2 years ago

This PR ...

Important changes which may need further discussion

Additional changes

stevehipwell commented 2 years ago

@HeinrichAD this PR has broken the Docker release action and we're now only getting generic tags. I suspect that it's due to the change in trigger in the new docker.yaml file breaking the release actions.

FYI @arnaudroques

HeinrichAD commented 2 years ago

I'm sorry to hear that. Unfortunately, my PC died today. Currently, I'm doing everything from my phone. I will need to bye a new one tomorrow und set it up. I'm sure that I will not be able to do much until next week.

But. The Action shows no error and hub.docker.com has the new images. Furthermore, I only export this workflow part into a own file and did not change the workflow (if I remember correctly).

Edit: I see. There is no tag with the new plantuml version on docker hub.

stevehipwell commented 2 years ago

@HeinrichAD you moved the Docker code into a workflow with a different trigger and it's a common pattern for an action to have different functionality based on the workflow trigger. I'm not sure why you moved the actions in the first place?

HeinrichAD commented 2 years ago

A workflow should not do everything. The main workflow creates all sources files and uploads them as release. The docker workflow is responsible for the docker related stuff. Troubleshooting is also simplified. Moreover, the former should not fail if the latter does not work.

Problem: workflow/docker.yml If I'm correct, the problem is that due to the changed trigger (workflow_run) the variable {{raw}} is not set correctly.

I see multiple solutions:

Each solution has its own advantages and disadvantages.

For the first idea you could use https://github.com/marketplace/actions/get-latest-tag to get the tag value and change e.g.: type=semver,pattern=jetty-{{raw}} to type=raw,value=jetty-{{steps.previoustag.outputs.tag}} (or what you name your variable)

@stevehipwell @arnaudroques I'm sorry but I can fix and test it at the moment by myself.

stevehipwell commented 2 years ago

@HeinrichAD I don't think that you have understood how GitHub actions are intended to be composed, specifically the workflow vs job part. A workflow is for a single group of related steps and can be further broken down into multiple jobs if required. It's a shame you didn't loop me in to your changes, as the last person to commit to the main workflow, as that would have saved this breaking.

Moving forward I suggest that you open a PR to reinstate the Docker actions back to the job in the main workflow. Once this is merged you can open a PR to split the Docker steps out into a separate job in the same workflow, which will allow the build to pass even if the Docker part fails. Just remember that each additional job (or workflow) increases both the cost (more VMs) and time (more setup and moving of artifacts) of the CI process.

arnaudroques commented 2 years ago

@HeinrichAD Take the time you need to set up your PC

@stevehipwell I am really far from my area of expertise here. I suggest that you and @HeinrichAD discuss together to see how to fix this. As last option, I can revert this PR but I think it would be a shame because there are interesting improvement in this PR. What do you think about it ?