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

chore(build): upgrade Go to 1.21.10, use custom builder instead of cross-builder #2819

Merged
merged 6 commits into from
Jun 4, 2024

Conversation

bednar
Copy link
Contributor

@bednar bednar commented May 28, 2024

Closes https://github.com/influxdata/edge/issues/685

This PR introduces several changes:

  1. Upgrades GoLang to 1.21.10.
  2. Upgrades the protobuf library to 3.18.3 and standardizes its use across the source base, including in Dockerfile_build, build.sh, and test.sh.

For more info see:

Custom builder

The reason for a new custom builder is compatibility issues between the protobuf library and Chronograf's UDFs for Python. The updated version of the cross-builder (see PR #669) upgrades protobuf to version 26.1. However, this version introduces breaking changes in the Python protobuf library. The Python build for protobuf, available at protobuf 5.26.1 on PyPI, does not support Python 2. As a result, updating to the latest version of protobuf (by using newest `cross-builder) would eliminate support for Python v2 in UDFs.

@bednar bednar marked this pull request as ready for review May 28, 2024 11:00
Copy link
Contributor

@davidby-influx davidby-influx left a comment

Choose a reason for hiding this comment

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

I'm going to defer to @bnpfeife 's review of this...

Copy link
Contributor

@bnpfeife bnpfeife left a comment

Choose a reason for hiding this comment

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

This looks good to me! It looks like there's no automation around building+pushing the custom builder docker image; however, since it's fairly straightforward to do the build+push from a developer machine, I'm not going to block on that.

Thank you for implementing this! My apologies for being absent from this effort, I've been swamped with several other projects.

@bednar
Copy link
Contributor Author

bednar commented May 30, 2024

@bnpfeife, thank you for your approval.

Copy link
Contributor

@jdstrand jdstrand left a comment

Choose a reason for hiding this comment

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

This LGTM overall. This changes quite a lot of how to build the image and to keep it up to date. Can you add some documentation on this? It should include, at a minimum

  • why we are using a custom builder
  • the general procedures for updating golang, rust and protobuf in the custom builder
  • when to update Dockerfile_build_ubuntu64
  • when to update .circleci/config.yml

This will help our future selves with maintenance. Thanks!

Dockerfile_build Outdated
#
# src: https://github.com/influxdata/edge/blob/4677c285014ac27727e5a1ae9bf2c1633afc6ea6/dockerfiles/cross-builder/install-rust.sh#L7
#
ENV RUST_LATEST_VERSION=1.63.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Rust is now at 1.78.0. Is it possible to update to that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rust has been updated to the latest version. I will prepare a builder/README.md file with documentation on how Kapacitor is built, including how to update builder components - Go, Rust, ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The builder/README.md is ready to review.

@bednar bednar requested a review from jdstrand June 3, 2024 10:37
@@ -11,4 +11,4 @@ DOCKER_TAG="kapacitor-$(date +%Y%m%d)"
docker build --rm=false --platform linux/amd64 -f ./Dockerfile_build -t builder:"$DOCKER_TAG" .
docker tag builder:"$DOCKER_TAG" quay.io/influxdb/builder:"$DOCKER_TAG"

docker push quay.io/influxdb/builder:"$DOCKER_TAG"
#docker push quay.io/influxdb/builder:"$DOCKER_TAG"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be uncommented, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, my apologies. I tested the script without pushing the new Docker image. It is fixed.

@bednar bednar requested a review from jdstrand June 3, 2024 14:15
Copy link
Contributor

@jdstrand jdstrand left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@bednar bednar merged commit 64e5d1d into master Jun 4, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants