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.

Sign up for GitHub

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

Jump to bottom

Added support for linux/arm64 images in helm-charts #348

Merged
merged 1 commit into from
Sep 14, 2023
Merged

Added support for linux/arm64 images in helm-charts #348

merged 1 commit into from
Sep 14, 2023

Conversation

prafull01
Copy link
Collaborator

@prafull01 prafull01 commented Sep 12, 2023 •
edited
Loading

Fixes #330

@prafull01 prafull01 requested review from udnay and rail September 12, 2023 04:34
rail
rail approved these changes Sep 12, 2023
View reviewed changes
build/docker-image/self-signer-cert-utility/Dockerfile Outdated
ADD https://binaries.cockroachdb.com/cockroach-v23.1.2.linux-amd64.tgz .
RUN tar -zxvf cockroach-v23.1.2.linux-amd64.tgz
RUN if [ "$TARGETPLATFORM" = "linux/amd64" ]; then GOARCH=amd64; elif [ "$TARGETPLATFORM" = "linux/arm64" ]; then GOARCH=arm64; else GOARCH=amd64; fi && \
curl -sS -L -O https://binaries.cockroachdb.com/cockroach-v23.1.2.linux-${GOARCH}.tgz && \
Copy link
Member

Choose a reason for hiding this comment

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

A nit. Can you update the crdb version to the latest one? As a bonus you can move it to a variable which can be passed in. Not a blocker though, feel free to do this as a separate PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did even better, now the self-signer automatically picks up the latest cockroach binary version defined in the Chart.yaml. Now, no need for special code changes to upgrade cockroach binary in docker image. Please have a look.

build/docker-image/self-signer-cert-utility/Dockerfile Outdated
@@ -18,13 +18,16 @@ COPY cmd/ cmd/
COPY pkg/ pkg/

# Build the binary self-signer utility
RUN go build -a -installsuffix cgo -o self-signer cmd/main.go
RUN go build -o self-signer cmd/main.go

# Install the cockroach CLI
Copy link
Member

Choose a reason for hiding this comment

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

A nit. "cockroach CLI" means cockroach-cli, which is a separate binary and doesn't contain any "server" code. Maybe adjust the comment to something like "Install the cockroach binary"?

bundle/manifests/cockroachdb.clusterserviceversion.yaml
@@ -706,3 +706,6 @@ spec:
name: Helm Community
url: https://artifacthub.io/packages/helm/cockroachdb/cockroachdb
version: VERSION
labels:
operatorframework.io/arch.amd64: supported
operatorframework.io/arch.arm64: supported
Copy link
Member

Choose a reason for hiding this comment

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

Yassss!!!!

mprimeaux
mprimeaux reviewed Sep 12, 2023
View reviewed changes
Copy link
Contributor

@mprimeaux mprimeaux left a comment

Choose a reason for hiding this comment

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

Thanks for these changes!

.github/workflows/build.yaml

- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v2

- name: Login to GCR
uses: docker/login-action@v1
Copy link
Contributor

Choose a reason for hiding this comment

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

The latest version of this GitHub action is v3. Do you prefer to upgrade from v1?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Upgrade to v3 will be done in separate PR.

.github/workflows/build.yaml
@@ -43,15 +43,18 @@ jobs:
- name: Checkout sources
uses: actions/checkout@v2

- name: Set up QEMU
uses: docker/setup-qemu-action@v2
Copy link
Contributor

Choose a reason for hiding this comment

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

The latest version of this GitHub action is v3. Do you prefer to upgrade from v2?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Upgrade to v3 will be done in separate PR and will be done in all github actions.

.github/workflows/build.yaml
uses: docker/setup-qemu-action@v2

- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v2
Copy link
Contributor

Choose a reason for hiding this comment

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

The latest version of this GitHub action is v3. Do you prefer to upgrade from v2?

build/docker-image/self-signer-cert-utility/Dockerfile
ADD https://binaries.cockroachdb.com/cockroach-v23.1.2.linux-amd64.tgz .
RUN tar -zxvf cockroach-v23.1.2.linux-amd64.tgz
# Install the cockroach binary
RUN if [ "$TARGETPLATFORM" = "linux/amd64" ]; then GOARCH=amd64; elif [ "$TARGETPLATFORM" = "linux/arm64" ]; then GOARCH=arm64; else GOARCH=amd64; fi && \
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a minor point but if you were to add TARGETOS and TARGETARCH args then you can set GOOS and GOARCH to those values, respectively. Those args are passed in by docker buildx (and by nerdctl build) intrinsically.

I usually include the following block in my "build" stage so I see what's going on a bit more. I'm not suggesting this change but just putting it here for awareness. You can see where I set GOOS and GOARCH.

ARG BUILDOS
ARG BUILDPLATFORM
ARG BUILDARCH
ARG BUILDVARIANT
ARG TARGETPLATFORM
ARG TARGETOS
ARG TARGETARCH
ARG TARGETVARIANT
ARG BUILD_VERSION
ARG BUILD_COMMIT

RUN echo "Container image build platform details :: "
RUN echo "BUILDPLATFORM.....: $BUILDPLATFORM"
RUN echo "BUILDOS...........: $BUILDOS"
RUN echo "BUILDARCH.........: $BUILDARCH"
RUN echo "BUILDVARIANT......: $BUILDVARIANT"
RUN echo "Container image target platform details :: "
RUN echo "TARGETPLATFORM....: $TARGETPLATFORM"
RUN echo "TARGETOS..........: $TARGETOS"
RUN echo "TARGETARCH........: $TARGETARCH"
RUN echo "TARGETVARIANT.....: $TARGETVARIANT"

ENV GOOS=$TARGETOS
ENV GOARCH=$TARGETARCH

cockroachdb/values.yaml
# Taints to be tolerated by the Pod of this Job.
# https://kubernetes.io/docs/concepts/configuration/taint-and-toleration/
tolerations: []

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding these!

mprimeaux
mprimeaux reviewed Sep 13, 2023
View reviewed changes
Copy link
Contributor

@mprimeaux mprimeaux left a comment

Choose a reason for hiding this comment

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

LGTM

@prafull01 prafull01 merged commit 31f8f1e into cockroachdb:master Sep 14, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mprimeaux mprimeaux mprimeaux left review comments

@rail rail rail approved these changes

@udnay udnay Awaiting requested review from udnay

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cockroach-self-signer-cert:1.4 is not multi-arch
3 participants
@prafull01 @rail @mprimeaux