-
Notifications
You must be signed in to change notification settings - Fork 140
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
Update operator-sdk and Go version #184
Update operator-sdk and Go version #184
Conversation
so neat, thank you for making that change, @VaishnaviHire . Now, with config/manager/manager.yaml present and OCP 4.10, it becomes much easier to make an operator bundle supporting a set of relatedImages, and without having to manually find out the sha256 digests. See https://docs.openshift.com/container-platform/4.10/operators/operator_sdk/osdk-generating-csvs.html#olm-enabling-operator-for-restricted-network_osdk-generating-csvs @Jooho |
4dfb521
to
aad0a9b
Compare
caf42ca
to
17b27da
Compare
17b27da
to
619b3cf
Compare
@VaishnaviHire: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Note: This PR is ready for reviews |
Dockerfile
Outdated
COPY go.mod . | ||
COPY go.sum . | ||
# Build the manager binary | ||
FROM golang:1.18 as builder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use one of the one of the ubi8/golang tool images?
I'm seeing a bunch at
❯ skopeo list-tags docker://registry.access.redhat.com/ubi8/go-toolset | grep 1.18
"1.18",
"1.18.4",
"1.14.12-17.1618436992-source",
"1.18.4-8.1669838000-source",
"1.14.12-17.1618436992",
"1.18.4-8-source",
"1.18.4-8.1669838000",
"1.18.4-8",
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VaishnaviHire Can you make the version a docker ARG
? Do we need to add the z-stream (1.18.9
) to minimize any rebuilds from breaking due to a go version update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you actually use registry.redhat.io/ubi8/go-toolset instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the Dockerfile with registry.redhat.io/ubi8/go-toolset
image with Go version as a docker ARG
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VaishnaviHire We will have to use registry.access.redhat.com/ubi8/go-toolset
for unauthenticated pulls.
Note: Updated the base branch to |
Dockerfile
Outdated
COPY go.mod . | ||
COPY go.sum . | ||
# Build the manager binary | ||
FROM golang:1.18 as builder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VaishnaviHire Can you make the version a docker ARG
? Do we need to add the z-stream (1.18.9
) to minimize any rebuilds from breaking due to a go version update.
619b3cf
to
5f6a531
Compare
5f6a531
to
5a688ef
Compare
5a688ef
to
bc79aab
Compare
Updated the PR with instructions to deploy Operator using OLM |
Makefile
Outdated
|
||
.PHONY: docker-build | ||
docker-build: manifests generate fmt vet ## Build docker image with the manager. | ||
docker build -t ${IMG} . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do this in a follow-up PR but we should move use an IMG_BUILDER
variable to allow someone to choose podman
or docker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also default to podman.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VaishnaviHire I can make this change in a follow-up PR if you want
Update dockerfile
ea8745a
to
c698e51
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Build and deploy worked perfectly for me without any issues. I created a new kfdef and the operator deployed the pods without any noticeable issues
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: LaVLaS The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
* Initialize operator-sdk 1.23 * Add apis from kfctl * Add KfDef controller * Add 'pkg' folder * Add Secret Generator controller * Update go modules and depricated methods * Remove b2ndcontroller, update watch functions * Update 'Apply()' to initalize ApplyOptions * Update manifests config * Update Dockerfile, Makefile & README Update dockerfile * Add Dashboard CRDs * Add OLM Bundle
* Initialize operator-sdk 1.23 * Add apis from kfctl * Add KfDef controller * Add 'pkg' folder * Add Secret Generator controller * Update go modules and depricated methods * Remove b2ndcontroller, update watch functions * Update 'Apply()' to initalize ApplyOptions * Update manifests config * Update Dockerfile, Makefile & README Update dockerfile * Add Dashboard CRDs * Add OLM Bundle
Signed-off-by: Wen Zhou <[email protected]>
Fixes #176
Description
How Has This Been Tested?
Merge criteria: