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.

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

Reusable build docker action #10017

Merged
merged 3 commits into from Aug 9, 2022
Merged

Conversation

npepinpe
Copy link
Member

@npepinpe npepinpe commented Aug 5, 2022

Description

This PR aggregates the steps required to build the Docker image into a reusable action called build-docker. The action can build and push the Docker image, as well as package the artifacts for it if need be.

Further iterations may want to split packaging out into its own action.

Its usage is as:

- uses: ./.github/actions/build-docker
  with:
    repository: localhost:5000/camunda/zeebe
    version: current-test
    push: true
    package: true

The action will set up Docker & Buildx, Java, and Maven. If package is true, it will also setup Go in order to build zbctl.

When push is true, the image is pushed to the given repository, with the tag being made of the repository and version. In the above, this would be equivalent to docker push localhost:5000/camunda/zeebe:current-test. If push is false, then the image is built and loaded into the local Docker registry only.

If package is true, then zbctl is compiled, and all the modules are built and installed to the local maven repository. Additionally, the distribution TAR ball is built. If it's false, then the distribution TAR ball is expected to already be present under dist/target/camunda-zeebe-<VERSION>-tar.gz.

If the version is omitted, it will be read from the Maven project.

Related issues

related to #10013 (as a follow up)

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

  • The changes are backwards compatibility with previous versions
  • If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/1.3) to the PR, in case that fails you need to create backports manually.

Testing:

  • There are unit/integration tests that verify all acceptance criterias of the issue
  • New tests are written to ensure backwards compatibility with further versions
  • The behavior is tested manually
  • The change has been verified by a QA run
  • The impact of the changes is verified by a benchmark

Documentation:

  • The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
  • New content is added to the release announcement
  • If the PR changes how BPMN processes are validated (e.g. support new BPMN element) then the Camunda modeling team should be informed to adjust the BPMN linting.

Please refer to our review guidelines.

@npepinpe npepinpe changed the base branch from main to 9940-oci-labels-release August 5, 2022 15:25
@npepinpe npepinpe force-pushed the np-docker-build-action branch 3 times, most recently from ce65ab9 to 4d8707a Compare August 5, 2022 15:50
@npepinpe
Copy link
Member Author

npepinpe commented Aug 5, 2022

One thing to note here, the action doesn't work on self hosted runners. It could be made to work by setting up buildx here (or already in our self hosted runner), like on the ubuntu-latest GitHub runner. I tried 5 minutes and it didn't work, and I figured it wasn't critical yet, but let me know.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2022

Test Results

   812 files  +  1     812 suites  +1   1h 38m 2s ⏱️ - 5m 52s
6 093 tests  - 36  6 082 ✔️  - 37  10 💤 ±0  1 +1 
6 273 runs   - 44  6 262 ✔️  - 45  10 💤 ±0  1 +1 

For more details on these failures, see this check.

Results for commit e76b855. ± Comparison against base commit 7f1134c.

♻️ This comment has been updated with latest results.

@npepinpe
Copy link
Member Author

npepinpe commented Aug 5, 2022

I still have to test the deploy, btw, but I could use a pair of eyes to validate the approach.

@oleschoenburg oleschoenburg self-requested a review August 9, 2022 07:14
@npepinpe npepinpe force-pushed the 9940-oci-labels-release branch 2 times, most recently from ef20069 to afad210 Compare August 9, 2022 07:20
Copy link
Member

@oleschoenburg oleschoenburg left a comment

Choose a reason for hiding this comment

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

Bummer that we can't use it on self-hosted runners. Do you think a second pair of eyes would help to set it up?

The overall approach looks good to me, it's exactly how I imagined it would look like. I can't think of anything that would be problematic or that could be improved.

@npepinpe npepinpe force-pushed the 9940-oci-labels-release branch 2 times, most recently from 930c2d2 to 51716bb Compare August 9, 2022 07:57
@npepinpe
Copy link
Member Author

npepinpe commented Aug 9, 2022

So we can make it work on self-hosted runners in two ways:

  1. Have buildx setup already, like with the GitHub hosted runners
  2. Install and setup buildx. I just tried using the provided action and it wasn't enough, and I just didn't want to spend more time there. It can definitely be a refinement if we need it, but I'm not sure yet that we need it.

Base automatically changed from 9940-oci-labels-release to main August 9, 2022 09:21
@npepinpe
Copy link
Member Author

npepinpe commented Aug 9, 2022

@oleschoenburg I think I got it to work on self-hosted runners. So our integration tests will also build it, and I've set it up so they will also package the application in the same step, but I feel that might be too opaque. I'm thinking we might want (but not here) to continue building reusable actions, e.g.

  • setup-zeebe (setup java, go, maven)
  • package-zeebe (build go, build java)
  • build-docker (build docker image from package-zeebe outputs)

I think these are the most common "blocks" we have everywhere. Benefits are centralizing the Maven/Java/Go versions and how the artifacts are built. Downside is things start being a little more "hidden" =/

@npepinpe
Copy link
Member Author

npepinpe commented Aug 9, 2022

There was a known flaky test, but the rest looks OK. Let me know what you think.

@npepinpe npepinpe marked this pull request as ready for review August 9, 2022 13:36
Copy link
Member

@oleschoenburg oleschoenburg left a comment

Choose a reason for hiding this comment

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

Awesome, looks good to me 👍
Just two minor comments.

.github/actions/build-docker/action.yml Show resolved Hide resolved
.github/actions/build-docker/action.yml Outdated Show resolved Hide resolved
@npepinpe
Copy link
Member Author

npepinpe commented Aug 9, 2022

bors merge

@zeebe-bors-camunda
Copy link
Contributor

Build succeeded:

@zeebe-bors-camunda zeebe-bors-camunda bot merged commit 77f9f1d into main Aug 9, 2022
@zeebe-bors-camunda zeebe-bors-camunda bot deleted the np-docker-build-action branch August 9, 2022 15:44
@npepinpe npepinpe mentioned this pull request Aug 15, 2022
10 tasks
zeebe-bors-camunda bot added a commit that referenced this pull request Aug 16, 2022
10048: Common reusable actions r=npepinpe a=npepinpe

## Description

This PR aggregates the steps to set up the Zeebe environment for packaging, as well as packaging itself, into two reusable actions. By doing this, it removes the duplicate code in the `build-docker` action, and instead relies on the caller having set up and packaged Zeebe beforehand (much like we rely on the checkout step being invoked before).

`setup-zeebe` will prepare the environment to build the complete Zeebe distribution. The benefits here is that we centralize once the tech stack, how it's installed, and the versions we require, instead of spreading this everywhere. I've made it configurable so some parts can be ignored, primarily because of the `setup-go` action. If you use that, but do not do anything with Go, it will fail in its post-run when trying to clean a non-existent cache 🤷 The action requires the code to have been checked out beforehand.

`build-zeebe` will build the Go client (optional) and the Java distribution (optional). By default, it builds everything. It also requires the environment to be set up beforehand, and the code checked out. It uses the `clients/go/cmd/zbctl/build.sh` script to build the Go client, and will `install` the mvn modules, skipping checks and using `-T1C` to parallelize by default. You can customize the installation via a free-form field letting you set up additional properties to pass to the build. The benefits here is again centralization of the common set up, while allowing enough customization for the various steps.

I decided to split both set up and packaging because there are some places where we package the application differently, notably when deploying/releasing. As this will most likely be used in the future for releases, it made sense to me. So the packaging is really more of a "package for non-release cases". It could be changed to allow us to modify the command for packaging (right now you can only pass options, but cannot change the actual goals to execute), and then I suppose it could be reused. I think it might be misleading though to have a build Zeebe action which actually deploys, so I'd rather avoid that for now.

## Related issues

related to #10017 



Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>
@Zelldon Zelldon added the version:8.1.0 Marks an issue as being completely or in parts released in 8.1.0 label Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
version:8.1.0-alpha5 Marks an issue as being completely or in parts released in 8.1.0-alpha5 version:8.1.0 Marks an issue as being completely or in parts released in 8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants