Refine the other operator related names #631

zhangtbj · 2021-03-01T13:11:24Z

Changes

After the PR Rename Shipwright components away from "operator": #610

There are still some codes and comments which uses operator which may make end user or new developer confusion.
If we decide to swtich build operator to Shipwright build controller, we should review all codes and switch them all.

Still keep operator-sdk related words as before
Modify the code and related properties, etc ...
Modify in the documents
Modify in the test codes
Update the version of the Shipwright build controller from 0.0.1 to 0.3.0

/kind cleanup

Submitter Checklist

[ n ] Includes tests if functionality changed/was added
[ n ] Includes docs if changes are user-facing
[ y ] Set a kind label on this PR
[ y ] Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

Renamed "build operator" to "build controller" in code and documentation.

openshift-ci-robot · 2021-03-01T13:11:26Z

@zhangtbj: The label(s) kind/misc cannot be applied, because the repository doesn't have them

In response to this:

Changes

There are still some codes and comments which uses operator which may make end user or new developer confusion.
If we decide to swtich build operator to Shipwright build controller, we should review all codes and switch them all.

Still keep operator-sdk related words as before

Modify the code and related properties, etc ...

Modify in the documents

Modify in the test codes

Update the version of the Shipwright build controller from 0.0.1 to 0.3.0

/kind misc

Submitter Checklist

Includes tests if functionality changed/was added

Includes docs if changes are user-facing

Set a kind label on this PR

Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

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.

zhangtbj · 2021-03-01T13:16:19Z

/kind cleanup

version/version.go

gabemontero · 2021-03-01T17:00:44Z

@zhangtbj you currently have a release-note set but with no text

was your intent to have a release note, or to not have one? Or is determining that part of the work in progress?

zhangtbj · 2021-03-02T04:55:09Z

Hi @gabemontero my mistake.

It doesn't require the release notes. Just the internal fix for the renaming.

I remove the release-notes label.

Thanks!

qu1queee · 2021-03-02T20:20:30Z

test/integration/utils/operator.go

 // but it disables the prometheus metrics and leader election. This intended
 // to for testing.
-func (t *TestBuild) StartBuildOperator() (chan struct{}, error) {
+func (t *TestBuild) StartBuildController() (chan struct{}, error) {


I think I did a similar comment somewhere else, this function actually starts a manager, which is an interface in the controller-runtime pkg. I think StartManager is a better name, rather than a search/replace approach.

Thanks!, two cents:

I think the logic should be: start a controller and controller starts a manager, so manager should be included in the controller, and the goal of this file is used to start a controller for test, not start a manager as a internal usage.

I am not sure if we want to add more config beside of manager in future to enrich the test.

So I prefer to keep StartBuildController.

But I am fine for both names.

Technically speaking this function does the following:

Add all controllers to the manager, so we add four

Starts all registered Controllers and blocks until the Stop channel is closed

For me StartBuildController is missleading, can we call this StartControllers?

+1 for renaming, either Manager or Controllers

Agree and fixed. Thanks all

qu1queee · 2021-03-02T20:23:34Z

Note: If possible it would be nice if #632 can go first, that PR should have higher priority in general, but no strong opinions if that doesnt happen. Thanks for this missing enhancements @zhangtbj

HeavyWombat · 2021-03-03T08:34:16Z

Note: If possible it would be nice if #632 can go first, that PR should have higher priority in general, but no strong opinions if that doesnt happen. Thanks for this missing enhancements @zhangtbj

@zhangtbj @qu1queee Quick question: Since we removed any "operator" references from the End to End test anyway in #632, we could also consider removing any changes done here in the test/e2e directory from this PR.

qu1queee · 2021-03-03T10:11:31Z

@HeavyWombat thats a question for @zhangtbj

HeavyWombat · 2021-03-03T10:56:02Z

.github/workflows/ci.yml

@@ -119,7 +119,7 @@ jobs:
            make install-controller-kind
            kubectl -n shipwright-build rollout status deployment shipwright-build-controller --timeout=1m || true
        - name: Test
-          run: TEST_E2E_OPERATOR=managed_outside TEST_NAMESPACE=shipwright-build TEST_IMAGE_REPO=registry.registry.svc.cluster.local:32222/shipwright-io/build-e2e make test-e2e
+          run: TEST_E2E_CONTROLLER=managed_outside TEST_NAMESPACE=shipwright-build TEST_IMAGE_REPO=registry.registry.svc.cluster.local:32222/shipwright-io/build-e2e make test-e2e


We tackled that in #632. Like commented somewhere else too, I would opt for leaving the end to end tests untouched in this PR here.

Thank you Matthias, agree. But it is hard to extract the change in multiple files. Let us merge the #632 PR first, then I will resolve the conflict here.

HeavyWombat · 2021-03-03T10:56:32Z

Makefile

+# E2E test controller behavior, can be start_local or managed_outside
+TEST_E2E_CONTROLLER ?= start_local


E2E related

Thanks Matthias, I think this PR can be wait until your PR is merged: #632

Then I can rebase and refactor the e2e related change later.

HeavyWombat · 2021-03-03T10:56:49Z

Makefile

 	TEST_WATCH_NAMESPACE=${TEST_NAMESPACE} \
-	TEST_E2E_OPERATOR=${TEST_E2E_OPERATOR} \
+	TEST_E2E_CONTROLLER=${TEST_E2E_CONTROLLER} \


E2E related

HeavyWombat · 2021-03-03T10:57:36Z

docs/development/testing.md

@@ -119,7 +119,7 @@ The following table contains a set of environment variables that control the beh
 | `TEST_NAMESPACE`                | `default`                                                                                        | Target namespace to execute tests upon, default is `default`. |
 | `TEST_E2E_FLAGS`                | `-failFast -flakeAttempts=2 -p -randomizeAllSpecs -slowSpecThreshold=300 -timeout=20m -trace -v` | Ginkgo flags. See all Ginkgo flags here: [The Ginkgo CLI](https://onsi.github.io/ginkgo/#the-ginkgo-cli). Especially of interest are `--focus` and `--skip` to run selective tests. |
 | `TEST_E2E_CREATE_GLOBALOBJECTS` | `true`                                                                                           | Boolean, if `false`, the custom resource definitions and (cluster) build strategies are not created and deleted by the e2e test code |
-| `TEST_E2E_OPERATOR`             | `start_local`                                                                                    | String with allowed values `start_local` (will start the local operator and print its logs add the end of the test run) and `managed_outside` (will assume that the operator is running through whatever means) |
+| `TEST_E2E_CONTROLLER`             | `start_local`                                                                                    | String with allowed values `start_local` (will start the local Shipwright build controller and print its logs add the end of the test run) and `managed_outside` (will assume that the Shipwright build controller is running through whatever means) |


E2E related

HeavyWombat · 2021-03-03T10:58:45Z

test/e2e/e2e_suite_test.go

@@ -17,7 +17,7 @@ import (
 )

 var (
-	operatorCmd *exec.Cmd
+	controllerCmd *exec.Cmd


E2E related

HeavyWombat · 2021-03-03T10:58:52Z

test/e2e/e2e_test.go

@@ -10,13 +10,13 @@ import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"

-	operator "github.com/shipwright-io/build/pkg/apis/build/v1alpha1"
+	controller "github.com/shipwright-io/build/pkg/apis/build/v1alpha1"


E2E related

HeavyWombat · 2021-03-03T10:59:44Z

test/integration/utils/operator.go

 // but it disables the prometheus metrics and leader election. This intended
 // to for testing.
-func (t *TestBuild) StartBuildOperator() (chan struct{}, error) {
+func (t *TestBuild) StartBuildController() (chan struct{}, error) {


+1 for renaming, either Manager or Controllers

HeavyWombat · 2021-03-03T11:00:37Z

test/integration/integration_suite_test.go

 	// so that we can nuke the instance later inside the AfterEach Ginkgo
 	// block
-	tb.StopBuildOperator, err = tb.StartBuildOperator()
+	tb.StopBuildController, err = tb.StartBuildController()
 	if err != nil {
 		fmt.Println("fail to start the Build powerful operator", err)


Suggested change

fmt.Println("fail to start the Build powerful operator", err)

fmt.Println("fail to start the powerful Build controllers", err)

Thanks Matthias!
Fixed

adambkaplan

Given that this PR is updating documentation, I request that a release note be added, such as:

Renamed "build operator" to "build controller" in code and documentation.

adambkaplan · 2021-03-04T23:22:36Z

version/version.go

-// Version describes the version of build
-var Version = "0.0.1"
+// Version describes the version of Shipwright build controller
+var Version = "devel"


I don't think we are overwriting this value in make build or make release. If not, we should do so via go flags.

Agree.

I changed it as a go flags in our main.go, now it can be set in the different release deployment, like:

➜ build git:(operator_refine) ✗ go run cmd/manager/main.go --version v0.3.0 {"level":"info","ts":1614919747.8687959,"logger":"build","msg":"Shipwright Build Controller Version: v0.3.0"} {"level":"info","ts":1614919747.868852,"logger":"build","msg":"Go Version: go1.14.13"} {"level":"info","ts":1614919747.868873,"logger":"build","msg":"Go OS/Arch: darwin/amd64"}

At the same time, I also remove the operator-sdk version in the printVersion func.

Thanks!

zhangtbj · 2021-03-08T05:42:31Z

Hi @HeavyWombat and @qu1queee ,

After the PR is merged: #632

I rebased this PR and remove the E2E related code change.

Please review again. Thanks!

HeavyWombat

/lgtm

qu1queee

/approve

openshift-ci-robot · 2021-03-11T08:37:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qu1queee

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:

~~OWNERS~~ [qu1queee]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

qu1queee · 2021-03-11T12:48:29Z

/retest

zhangtbj · 2021-03-11T13:14:13Z

/retest

zhangtbj · 2021-03-11T13:15:46Z

Seems the ci/prow/4.4-unit test hangs, but I cannot see the logs or the content in the link (network issue), is there any way to rerun or restart it?

qu1queee · 2021-03-12T11:06:56Z

/retest

openshift-ci-robot added release-note Label for when a PR has specified a release note do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 1, 2021

openshift-ci-robot requested review from HeavyWombat and qu1queee March 1, 2021 13:11

openshift-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Mar 1, 2021

imjasonh reviewed Mar 1, 2021

View reviewed changes

version/version.go Outdated Show resolved Hide resolved

zhangtbj changed the title ~~WIP: Refine the other operator related names~~ Refine the other operator related names Mar 2, 2021

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 2, 2021

zhangtbj removed the release-note Label for when a PR has specified a release note label Mar 2, 2021

gabemontero added the release-note-none Label for when a PR does not need a release note label Mar 2, 2021

HeavyWombat mentioned this pull request Mar 2, 2021

Remove operator-SDK package pkg/test in End2End test #632

Merged

4 tasks

qu1queee requested changes Mar 2, 2021

View reviewed changes

HeavyWombat requested changes Mar 3, 2021

View reviewed changes

adambkaplan requested changes Mar 4, 2021

View reviewed changes

Refine the other operator related names

e3c34fc

HeavyWombat self-requested a review March 9, 2021 13:59

HeavyWombat approved these changes Mar 9, 2021

View reviewed changes

openshift-ci-robot assigned HeavyWombat Mar 9, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 9, 2021

qu1queee self-requested a review March 9, 2021 14:43

qu1queee approved these changes Mar 11, 2021

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 11, 2021

openshift-merge-robot merged commit 4ba23af into shipwright-io:master Mar 12, 2021

		# E2E test controller behavior, can be start_local or managed_outside
		TEST_E2E_CONTROLLER ?= start_local

	fmt.Println("fail to start the Build powerful operator", err)
	fmt.Println("fail to start the powerful Build controllers", err)

Refine the other operator related names #631

Refine the other operator related names #631

Conversation

zhangtbj commented Mar 1, 2021 • edited Loading

Changes

Submitter Checklist

Release Notes

openshift-ci-robot commented Mar 1, 2021

Changes

Submitter Checklist

Release Notes

zhangtbj commented Mar 1, 2021

gabemontero commented Mar 1, 2021

zhangtbj commented Mar 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qu1queee commented Mar 2, 2021

HeavyWombat commented Mar 3, 2021

qu1queee commented Mar 3, 2021

Choose a reason for hiding this comment

zhangtbj Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adambkaplan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangtbj commented Mar 8, 2021

HeavyWombat left a comment

Choose a reason for hiding this comment

qu1queee left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Mar 11, 2021

qu1queee commented Mar 11, 2021

zhangtbj commented Mar 11, 2021

zhangtbj commented Mar 11, 2021

qu1queee commented Mar 12, 2021

zhangtbj commented Mar 1, 2021 •

edited

Loading

zhangtbj Mar 5, 2021 •

edited

Loading