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

Enable local selection of dependencies #83

Merged
merged 1 commit into from
Mar 8, 2024
Merged

Enable local selection of dependencies #83

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions .github/workflows/golang.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,13 @@ jobs:
steps:
- uses: actions/checkout@v4
name: Checkout code

- name: Build
run: make docker-build
- name: Get Golang version
id: vars
run: |
GOLANG_VERSION=$( grep "GOLANG_VERSION ?=" versions.mk )
echo "GOLANG_VERSION=${GOLANG_VERSION##GOLANG_VERSION ?= }" >> $GITHUB_ENV
- name: Install Go
uses: actions/setup-go@v5
with:
go-version: ${{ env.GOLANG_VERSION }}
- run: make build
ArangoGutierrez marked this conversation as resolved.
Show resolved Hide resolved
15 changes: 14 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,13 @@ CMDS := $(patsubst ./cmd/%/,%,$(sort $(dir $(wildcard ./cmd/*/))))
CMD_TARGETS := $(patsubst %,cmd-%, $(CMDS))

CHECK_TARGETS := golangci-lint
MAKE_TARGETS := binaries build check fmt lint-internal test examples cmds coverage generate vendor check-vendor $(CHECK_TARGETS)
MAKE_TARGETS := binaries build build-image check fmt lint-internal test examples cmds coverage generate vendor check-vendor $(CHECK_TARGETS)

TARGETS := $(MAKE_TARGETS) $(CMD_TARGETS)

DOCKER_TARGETS := $(patsubst %,docker-%, $(TARGETS))
.PHONY: $(TARGETS) $(DOCKER_TARGETS)
DOCKERFILE_DEVEL := $(CURDIR)/deployments/container/Dockerfile.devel
Copy link
Member

Choose a reason for hiding this comment

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

Could we move this Dockerfile. It is NOT related to deploying this component. It is used to build the go binaries locally.


GOOS ?= linux
ifeq ($(VERSION),)
Expand Down Expand Up @@ -139,6 +140,18 @@ generate-clientset: .remove-clientset .remove-deepcopy .remove-crds
.remove-clientset:
rm -rf $(CURDIR)/$(PKG_BASE)/clientset

build-image: $(DOCKERFILE_DEVEL)
$(DOCKER) build \
--progress=plain \
--build-arg GOLANG_VERSION="$(GOLANG_VERSION)" \
--build-arg CLIENT_GEN_VERSION="$(CLIENT_GEN_VERSION)" \
--build-arg CONTROLLER_GEN_VERSION="$(CONTROLLER_GEN_VERSION)" \
--build-arg GOLANGCI_LINT_VERSION="$(GOLANGCI_LINT_VERSION)" \
--build-arg MOQ_VERSION="$(MOQ_VERSION)" \
klueska marked this conversation as resolved.
Show resolved Hide resolved
--tag $(BUILDIMAGE) \
-f $(DOCKERFILE_DEVEL) \
.
ArangoGutierrez marked this conversation as resolved.
Show resolved Hide resolved

$(DOCKER_TARGETS): docker-%:
@echo "Running 'make $(*)' in container image $(BUILDIMAGE)"
$(DOCKER) run \
Expand Down
29 changes: 29 additions & 0 deletions deployments/container/Dockerfile.devel
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

I don't agree with the mix that we have here. We now have this Dockerfile defined in two places -- the k8s-test-infra repo and here with no clear dependencies defined between the two. What causes each of these to change? How do we ensure that these are kept in sync.

The original motivation for the k8s-test-infra image was to have a single image to build these components and to be able to reuse that in the CI steps. At the moment, however, we seemt to have moved to native Golang steps -- with the exception of the build step which could also be native in this case.

Could we rather switch to using local images and not depend on k8s-test-infra at all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The approach on this Patch was to enable a fall-back option for local development, to enable a developer to try new versions of the same dependencies at will in real-time, without requiring a PR against k8s-test-infra to bump a dependency there. I will file a new patch to propose a better solution that still gives us the flexibility of this patch, but centralizes everything at k8s-test-infra

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#86

Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# Copyright (c) 2024, NVIDIA CORPORATION. All rights reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
ARG GOLANG_VERSION=x.x.x
FROM golang:${GOLANG_VERSION}

ARG CLIENT_GEN_VERSION=v0.26.1
ARG CONTROLLER_GEN_VERSION=v0.9.2
ARG GOLANGCI_LINT_VERSION=v1.52.0
ARG MOQ_VERSION=v0.3.4

RUN go install sigs.k8s.io/controller-tools/cmd/controller-gen@${CONTROLLER_GEN_VERSION} \
&& go install k8s.io/code-generator/cmd/client-gen@${CLIENT_GEN_VERSION} \
&& go install github.com/golangci/golangci-lint/cmd/golangci-lint@${GOLANGCI_LINT_VERSION} \
&& go install github.com/matryer/moq@${MOQ_VERSION}

# We need to set the /work directory as a safe directory.
# This allows git commands to run in the container.
RUN git config --file=/.gitconfig --add safe.directory /work
9 changes: 7 additions & 2 deletions versions.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,12 @@ vVERSION := v$(VERSION:v%=%)
GOLANG_VERSION ?= 1.20.4
CUDA_VERSION ?= 11.8.0

BUILDIMAGE_TAG ?= devel-go$(GOLANG_VERSION)
BUILDIMAGE ?= ghcr.io/nvidia/k8s-test-infra:$(BUILDIMAGE_TAG)
# these variables are only needed when building a local image
CLIENT_GEN_VERSION ?= v0.26.1
CONTROLLER_GEN_VERSION ?= v0.9.2
ArangoGutierrez marked this conversation as resolved.
Show resolved Hide resolved
GOLANGCI_LINT_VERSION ?= v1.52.0
MOQ_VERSION ?= v0.3.4
Comment on lines +28 to +31
Copy link
Collaborator

Choose a reason for hiding this comment

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

As mentioned before, I would drop these from here and they shouldn't really be configurable on a given branch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Retracting, given my coment above.

Copy link
Member

Choose a reason for hiding this comment

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

I would still remove them from here and only specify them in the Dockerfile. What motivation is there for keeping them defined here?

If these are "real" dependencies, then we should manage them using something else so that we can better manage the update lifecycle.


BUILDIMAGE ?= ghcr.io/nvidia/k8s-test-infra:devel-go$(GOLANG_VERSION)

GIT_COMMIT ?= $(shell git describe --match="" --dirty --long --always --abbrev=40 2> /dev/null || echo "")