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

NE-1802: Bump Golang, k8s.io, OpenShift API, and controller-runtime #225

Merged
merged 5 commits into from
Sep 17, 2024
Merged

NE-1802: Bump Golang, k8s.io, OpenShift API, and controller-runtime #225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7d07276
NE-1802: Bump Golang, k8s.io, OpenShift API, and controller-runtime
alebedev87 Aug 13, 2024
fbf0a43
golangci-lint: Bump to latest to fix high memory consumption
alebedev87 Aug 13, 2024
c544748
Remove unused Dockerfile.openshift
alebedev87 Aug 16, 2024
d11060f
e2e: increase DNS polling timeout to 7 minutes
alebedev87 Aug 28, 2024
f1b2155
manifests: bump operator image to latest(42c9f6d6b01d)
alebedev87 Sep 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
2 changes: 1 addition & 1 deletion .ci-operator.yaml
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry if I have missed a conversation about this, but doesn't splitting the bump & the vendor into two separate commits make the bump commit "broken"?

It could be just my personal philosophy, but I thought best practice was to avoid creating commits that don't compile. Was the motivation just to make reviews easier? I'm open to differing opinions, feel free to disagree, just curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reminded me of this discussion we had in C-I-O. I think I keep trying to stick to what I said in the discussion: put the vendor dir into a dedicated commit if either is true:

  • it's more convenient for the reviewer
  • the PR will be backported (vendored files may cause a lot of cherry-pick conflicts)

Was the motivation just to make reviews easier?

Yes. Will rebase the PR (fixup the vendor commit) once ready to move on.

Copy link
Contributor

Choose a reason for hiding this comment

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

There might be a small miscommunication, in the CIO discussion, I was referring to splitting out the vendor & bump commit as one together. I referred to that as the "vendor commit" there.

But in this PR, we have a "bump commit" and "vendor dir commit" separate. By bump commit, I mean the go get ... && go mod tidy commands (b871828). By "vendor dir commit", I mean go mod vendor (67ec0f2).

Will rebase the PR (fixup the vendor commit) once ready to move on.

What does that mean exactly? Are you saying your going to combine your b871828 67ec0f2 commits before merging?

Copy link
Contributor Author

@alebedev87 alebedev87 Aug 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There might be a small miscommunication, in the openshift/cluster-ingress-operator#1097 (comment), I was referring to splitting out the vendor & bump commit as one together. I referred to that as the "vendor commit" there.

But in this PR, we have a "bump commit" and "vendor dir commit" separate. By bump commit, I mean the go get ... && go mod tidy commands (b871828). By "vendor dir commit", I mean go mod vendor (67ec0f2).

Got it now. Yes, I understood the vendor commit as a commit with vendor dir only.

Will rebase the PR (fixup the vendor commit) once ready to move on.

What does that mean exactly? Are you saying your going to combine your b871828 67ec0f2 commits before merging?

Yes, will leave the PR will 2 commits: "bump+vendor dir" and "golangci-lint commit".

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay I think we are on the same page now. Thanks!

Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
build_root_image:
name: release
namespace: openshift
tag: rhel-8-release-golang-1.19-openshift-4.12
tag: rhel-8-release-golang-1.22-openshift-4.17
Copy link

Choose a reason for hiding this comment

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

Should you be using rhel-9 in the name instead of rhel-8 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought of postponing the migration to RHEL9 to not put all the eggs into one basket (rebase from upstream is huge plus this bump turned into a big one). Let's see if we'll have enough time to do RHEL9 migration too. But anyway it will be in a dedicated PR.

10 changes: 6 additions & 4 deletions .golangci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ run:
concurrency: 4

# timeout for analysis, e.g. 30s, 5m, default is 1m
timeout: 1m
timeout: 10m

# exit code when at least one issue was found, default is 1
issues-exit-code: 1
Expand All @@ -24,8 +24,9 @@ run:
# from this option's value (see skip-dirs-use-default).
# "/" will be replaced by current OS file path separator to properly work
# on Windows.
skip-dirs:
- vendor
issues:
exclude-dirs:
- vendor

# default is true. Enables skipping of directories:
# vendor$, third_party$, testdata$, examples$, Godeps$, builtin$
Expand All @@ -50,7 +51,8 @@ run:
output:
# colored-line-number|line-number|json|tab|checkstyle|code-climate|junit-xml|github-actions
# default is "colored-line-number"
format: tab
formats:
- format: tab

# print lines of code with issue, default is true
print-issued-lines: true
Expand Down
2 changes: 1 addition & 1 deletion Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# Build the manager binary
FROM golang:1.19 as builder
FROM golang:1.22 as builder

WORKDIR /opt/app-root/src
COPY . .
Expand Down
13 changes: 0 additions & 13 deletions Dockerfile.openshift
View file
Open in desktop

This file was deleted.

8 changes: 4 additions & 4 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ endif

CONTROLLER_GEN := go run sigs.k8s.io/controller-tools/cmd/controller-gen
SETUP_ENVTEST := go run sigs.k8s.io/controller-runtime/tools/setup-envtest
KUSTOMIZE := go run sigs.k8s.io/kustomize/kustomize/v4
K8S_ENVTEST_VERSION := 1.21.4
KUSTOMIZE := go run sigs.k8s.io/kustomize/kustomize/v5
K8S_ENVTEST_VERSION := 1.30.0
Copy link

Choose a reason for hiding this comment

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

Any reason to use 1.30.0 instead of 1.30.3 as is used in the go modules?

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. sigs.k8s.io/controller-runtime/tools/setup-envtest gets the artifacts from kubebuilder-tools which used to be published from the kubebuilder repository. Here is the link to all the kubebuilder tools releases available which I found in their latest release. There is no 1.30.3 release, that why I'm getting an error when I'm trying to use 1.30.3:

$ make test
go run sigs.k8s.io/controller-tools/cmd/controller-gen "crd:preserveUnknownFields=false" rbac:roleName=external-dns-operator webhook paths="./..." output:crd:artifacts:config=config/crd/bases
go run sigs.k8s.io/controller-tools/cmd/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..."
go fmt ./...
go vet ./...
unable to fetch hash for requested version: unable fetch metadata for kubebuilder-tools-1.30.3-linux-amd64.tar.gz -- got status "404 Not Found" from GCS

I think that we'll have to upgrade to the latest version of sigs.k8s.io/controller-runtime/tools/setup-envtest and start fetching the envtest binaries from the new location. The problem is that we cannot do it right now as the latest version uses 1.31.z k8s.io dependencies. So we have to postpone it to the next API bump.


PACKAGE=github.com/openshift/external-dns-operator

Expand Down Expand Up @@ -112,7 +112,7 @@ vet: ## Run go vet against code.
ENVTEST_ASSETS_DIR ?= $(shell pwd)/testbin
test: manifests generate fmt vet ## Run tests.
mkdir -p "$(ENVTEST_ASSETS_DIR)"
KUBEBUILDER_ASSETS="$(shell $(SETUP_ENVTEST) use "$(K8S_ENVTEST_VERSION)" --print path --bin-dir "$(ENVTEST_ASSETS_DIR)")" go test ./... -race -covermode=atomic -coverprofile coverage.out
KUBEBUILDER_ASSETS="$(shell $(SETUP_ENVTEST) use "$(K8S_ENVTEST_VERSION)" --print path --bin-dir "$(ENVTEST_ASSETS_DIR)")" CGO_ENABLED=1 go test ./... -race -covermode=atomic -coverprofile coverage.out

.PHONY: test-e2e
test-e2e:
Expand Down Expand Up @@ -218,7 +218,7 @@ endef
.PHONY: lint
## Checks the code with golangci-lint
lint: $(GOLANGCI_LINT_BIN)
$(GOLANGCI_LINT_BIN) run -c .golangci.yaml --deadline=30m
Copy link

Choose a reason for hiding this comment

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

Question - is there another way to give this a time limit, now that the deadline flag is deprecated?

Copy link
Contributor Author

@alebedev87 alebedev87 Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. It's already in the golangci-lint config. I don't see the reason to limit in 2 places, so I removed the flag completely. Also, 30m was an overkill, this test should not take more than a couple of seconds.

Upd: maybe a "couple of seconds" was a wrong estimation (failed job). Increased to 10 minutes.

$(GOLANGCI_LINT_BIN) run -c .golangci.yaml

$(GOLANGCI_LINT_BIN):
mkdir -p $(BIN_DIR)
Expand Down
18 changes: 12 additions & 6 deletions api/v1beta1/webhook_suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ import (
"sigs.k8s.io/controller-runtime/pkg/envtest"
logf "sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/log/zap"
metrics "sigs.k8s.io/controller-runtime/pkg/metrics/server"
"sigs.k8s.io/controller-runtime/pkg/webhook"
)

// These tests use Ginkgo (BDD-style Go testing framework). Refer to
Expand Down Expand Up @@ -85,12 +87,16 @@ var _ = BeforeSuite(func() {
// start webhook server using Manager
webhookInstallOptions := &testEnv.WebhookInstallOptions
mgr, err := ctrl.NewManager(cfg, ctrl.Options{
Scheme: scheme,
Host: webhookInstallOptions.LocalServingHost,
Port: webhookInstallOptions.LocalServingPort,
CertDir: webhookInstallOptions.LocalServingCertDir,
LeaderElection: false,
MetricsBindAddress: "0",
Scheme: scheme,
WebhookServer: webhook.NewServer(webhook.Options{
Host: webhookInstallOptions.LocalServingHost,
Port: webhookInstallOptions.LocalServingPort,
CertDir: webhookInstallOptions.LocalServingCertDir,
}),
LeaderElection: false,
Metrics: metrics.Options{
BindAddress: "0",
},
})
Expect(err).NotTo(HaveOccurred())

Expand Down
8 changes: 4 additions & 4 deletions api/v1beta1/webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import (
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/utils/pointer"
"k8s.io/utils/ptr"
)

func makeExternalDNS(name string, domains []ExternalDNSDomain) *ExternalDNS {
Expand Down Expand Up @@ -139,7 +139,7 @@ var _ = Describe("ExternalDNS admission webhook", func() {
{
ExternalDNSDomainUnion: ExternalDNSDomainUnion{
MatchType: DomainMatchTypeRegex,
Pattern: pointer.String(`(.*\.test.com`),
Pattern: ptr.To[string](`(.*\.test.com`),
},
FilterType: FilterTypeInclude,
},
Expand All @@ -166,14 +166,14 @@ var _ = Describe("ExternalDNS admission webhook", func() {
{
ExternalDNSDomainUnion: ExternalDNSDomainUnion{
MatchType: DomainMatchTypeExact,
Name: pointer.String("abc.test.com"),
Name: ptr.To[string]("abc.test.com"),
},
FilterType: FilterTypeInclude,
},
{
ExternalDNSDomainUnion: ExternalDNSDomainUnion{
MatchType: DomainMatchTypeExact,
Name: pointer.String(`(.*)\.test\.com`),
Name: ptr.To[string](`(.*)\.test\.com`),
},
FilterType: FilterTypeInclude,
},
Expand Down
2 changes: 1 addition & 1 deletion bundle/manifests/external-dns-operator.clusterserviceversion.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ spec:
fieldRef:
fieldPath: metadata.namespace
- name: RELATED_IMAGE_EXTERNAL_DNS
value: quay.io/external-dns-operator/external-dns@sha256:51e2acc81c804468932581379d674273fd2e62798be4e81c5c735bf0161590dd
value: quay.io/external-dns-operator/external-dns@sha256:42c9f6d6b01d5e45b7d5064d2d6dea1f7b51346198d80e7f7f9821bd7fd072cf
- name: TRUSTED_CA_CONFIGMAP_NAME
image: quay.io/openshift/origin-external-dns-operator:latest
name: external-dns-operator
Expand Down
4 changes: 4 additions & 0 deletions bundle/manifests/externaldns.olm.openshift.io_externaldnses.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,11 +311,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down Expand Up @@ -789,11 +791,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down
4 changes: 4 additions & 0 deletions config/crd/bases/externaldns.olm.openshift.io_externaldnses.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,11 +313,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down Expand Up @@ -791,11 +793,13 @@ spec:
items:
type: string
type: array
x-kubernetes-list-type: atomic
required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
matchLabels:
additionalProperties:
type: string
Expand Down
6 changes: 3 additions & 3 deletions config/manager/manager.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@ spec:
fieldRef:
fieldPath: metadata.namespace
- name: RELATED_IMAGE_EXTERNAL_DNS
# openshift/external-dns commit: 441588e482514db7722f5c793cf70348b68d952d
# manifest link: https://quay.io/repository/external-dns-operator/external-dns/manifest/sha256:51e2acc81c804468932581379d674273fd2e62798be4e81c5c735bf0161590dd
value: quay.io/external-dns-operator/external-dns@sha256:51e2acc81c804468932581379d674273fd2e62798be4e81c5c735bf0161590dd
# openshift/external-dns commit: 8da2509b922d50ef7b1b8ea2297758888f32448d
# manifest link: https://quay.io/repository/external-dns-operator/external-dns/manifest/sha256:42c9f6d6b01d5e45b7d5064d2d6dea1f7b51346198d80e7f7f9821bd7fd072cf
value: quay.io/external-dns-operator/external-dns@sha256:42c9f6d6b01d5e45b7d5064d2d6dea1f7b51346198d80e7f7f9821bd7fd072cf
- name: TRUSTED_CA_CONFIGMAP_NAME
securityContext:
capabilities:
Expand Down
Loading