Target Namespace #10

otaviof · 2021-05-17T13:36:31Z

Changes

Allowing namespaces to be configurable via .spec.namespace attribute, when not informed it uses shipwright-build by default.

---
apiVersion: operator.shipwright.io/v1alpha1
kind: ShipwrightBuild
metadata:
  name: shipwright-operator
spec:
  targetNamespace: default

There is some refactoring as well, notably implementing Finalizer Pattern in order to detect resource deletion, and handle manifests removal at Reconciler level.

Fixes #6

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

Release Notes

- Introducing `.spec.namespace`, the target namespace where Shipwright-Controller is deployed;

otaviof · 2021-05-21T06:31:16Z

/retitle Target Namespace

gabemontero

a brief/initial high level pass @otaviof .... no specific requests for change

I do pose a question around start up options ... curious on the opinions of you and the rest of the team.

FWIW the finalizer stuff is simpler than what we have to do with the CVO opeartors (where they have support the recreate behavior when managed). Not a bad thing of course.

I'm unfamiliar with the manifest stuff. So nothing to add there.

README.md

test/common.go

adambkaplan

Most of my comments are style nits and API doc cleanups.

I am concerned about the use of a finalizer - see the comment within the review.

README.md

controllers/result.go

controllers/shipwrightbuild_controller.go

adambkaplan · 2021-06-08T19:56:37Z

hack/test-with-envtest.sh

@@ -11,4 +11,4 @@ source "${ENVTEST_ASSETS_DIR}/setup-envtest.sh"
 fetch_envtest_tools "${ENVTEST_ASSETS_DIR}"
 setup_envtest_env "${ENVTEST_ASSETS_DIR}"
 # Run tests sequentially - the controller integration tests cannot be run concurrently
-go test ./... -coverprofile cover.out -p 1
+go test ./... -coverprofile cover.out -p 1 -failfast -ginkgo.v -ginkgo.failFast


You can pass -ginkgo flags this way? 🤯

api/v1alpha1/shipwrightbuild_types.go

adambkaplan · 2021-06-08T20:15:36Z

controllers/shipwrightbuild_controller.go


 	"github.com/shipwright-io/operator/api/v1alpha1"
 )

+const (
+	// FinalizerAnnotation annotation string appended on finalizer slice.
+	FinalizerAnnotation = "finalizer.operator.shipwright.io"


My main question is why do we need a finalizer? Finalizers make it difficult to tear down a Kubernetes cluster because it will block a namespace from getting deleted.

Is the concern that an admin will delete the target namespace by accident, and we'll get caught in this weird place where the operator won't work?

I have been researching this subject, and indeed, back in ~2018 we had issues open to deal with this situation. So, I have tested this with an updated cluster, and you can delete a namespace without getting stuck. it might have happen before, but not by design.

README.md

api/v1alpha1/shipwrightbuild_types.go

otaviof · 2021-07-01T06:47:26Z

@adambkaplan please consider it again with targetNamespace in place. Thanks in advance.

coreydaley · 2021-07-27T15:58:02Z

api/v1alpha1/shipwrightbuild_types.go

-	// Foo is an example field of ShipwrightBuild. Edit ShipwrightBuild_types.go to remove/update
-	Foo string `json:"foo,omitempty"`
+	// TargetNamespace is the target namespace where Shipwright's build controller will be deployed.
+	TargetNamespace string `json:"targetNamespace,omitempty"`


Can this just be Namespace?

targetNamespace is a common convention for operator configuration custom resources. The operator itself can be installed in a different namespace - for example, upstream with OLM operators will get installed by default in the olm-operators namespace. The community has converged on "target namespace" to indicate where cluster-scoped operand components are created.

This is different from, say, a MySQL operator that could provision multiple MySQL databases in multiple namespaces. For those the configuration CR would be namespace scoped, and the operands would be the individual MySQL DBs.

coreydaley · 2021-07-27T16:03:18Z

README.md

+```
+
+It will deploy the Build-Controller in `default` namespace. When `.spec.namespace` is not set,
+it will use the `shipwright-build` namespace instead, this namespace needs to be created before the


Have we already settled on the shipwright-build namespace somewhere or could we use something like just shipwright or shipwright-io ?

Given what this operator is going to rollout, I think shipwright-build might be a meaningful name. It's a good start in my opinion.

Not a bad thing to bike shed. Opened #15 so we can discuss without blocking this PR.

adambkaplan

/approve

This generally looks good @otaviof - please squash your commits and then another reviewer can lgtm.

openshift-ci · 2021-07-29T13:20:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan

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~~ [adambkaplan]

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

adambkaplan · 2021-07-29T13:56:52Z

/assign @coreydaley

Since you took a pass before, want to give this another look?

coreydaley · 2021-07-30T16:43:41Z

README.md

+metadata:
+  name: shipwright-operator
+spec:
+  namespace: default


can we go ahead and use shipwright-build in the example here since it is the default? And maybe add the yaml for creating the namespace to this example and make it a List?

I'm updating this example to use the shipwright-build namespace instead, and also showing the namespace must be present before. Please consider latest changes.

coreydaley · 2021-07-30T16:55:23Z

controllers/default_test.go

+	// namespace where ShipwrightBuild instance will be located
+	const namespace = "default"
+	// namespace where shipwright Controller and dependencies will be located
+	const targetNamespace = "namespace"
+


Can we use a namespace and targetNamespace that are more realistic and less confusing here?

Sure! I'm following your suggestion, please consider.

coreydaley · 2021-07-30T16:58:00Z

controllers/shipwrightbuild_controller.go

+	// default namespace instead
+	targetNamespace := b.Spec.Namespace
+	if targetNamespace == "" {
+		logger.Info("Namespace is not informed, using default namespace instead")


We should refer to the constant here instead of a hard coded shipwright-build

coreydaley · 2021-07-30T17:00:54Z

controllers/shipwrightbuild_controller_test.go

+	o "github.com/onsi/gomega"
+	"github.com/shipwright-io/operator/api/v1alpha1"
+	appsv1 "k8s.io/api/apps/v1"
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/log/zap"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"


Should be sorted into blocks

Okay, I'm adding a extra plugin to my editor, so that should happen automatically from now on 😅. Please consider latest changes.

CRD changes, adding `.spec.namespace` attribute. Deploying Shipwright Build Controller, and dependencies, on a target namespace informed on the CRD spec. Additionally, implementing Kubernetes Finalizer workflow to remove objects with actions centered on the Reconciler directly. Co-authored-by: Adam Kaplan <[email protected]>

coreydaley · 2021-08-03T14:32:55Z

/lgtm

openshift-ci bot added release-note do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels May 17, 2021

openshift-ci bot requested review from gabemontero and imjasonh May 17, 2021 13:36

otaviof added the kind/feature Categorizes issue or PR as related to a new feature. label May 17, 2021

otaviof force-pushed the issue-6 branch 2 times, most recently from a42f6b8 to 73fd05e Compare May 17, 2021 14:09

otaviof mentioned this pull request May 20, 2021

Refactoring Flags #11

Merged

4 tasks

otaviof force-pushed the issue-6 branch 2 times, most recently from 1e2f016 to 5934b71 Compare May 20, 2021 12:47

openshift-ci bot changed the title ~~[WIP] Target Namespace~~ Target Namespace May 21, 2021

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

gabemontero reviewed May 21, 2021

View reviewed changes

README.md Show resolved Hide resolved

imjasonh reviewed Jun 3, 2021

View reviewed changes

test/common.go Outdated Show resolved Hide resolved

otaviof force-pushed the issue-6 branch from 5b6a4da to 67218f1 Compare June 4, 2021 05:11

otaviof requested review from adambkaplan and alicerum June 8, 2021 15:59

adambkaplan requested changes Jun 8, 2021

View reviewed changes

otaviof force-pushed the issue-6 branch 4 times, most recently from 6fa8605 to 58d1587 Compare June 9, 2021 12:16

otaviof requested a review from adambkaplan June 9, 2021 12:23

adambkaplan reviewed Jun 30, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

api/v1alpha1/shipwrightbuild_types.go Outdated Show resolved Hide resolved

otaviof force-pushed the issue-6 branch from f3f812e to d021388 Compare July 1, 2021 06:43

otaviof requested a review from adambkaplan July 1, 2021 06:45

otaviof force-pushed the issue-6 branch from d021388 to d5bc251 Compare July 27, 2021 14:33

coreydaley reviewed Jul 27, 2021

View reviewed changes

adambkaplan mentioned this pull request Jul 29, 2021

Change default operand namespace to shipwright #15

Open

adambkaplan approved these changes Jul 29, 2021

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 29, 2021

otaviof force-pushed the issue-6 branch from d5bc251 to 15400d2 Compare July 29, 2021 13:53

openshift-ci bot assigned coreydaley Jul 29, 2021

coreydaley suggested changes Jul 30, 2021

View reviewed changes

otaviof force-pushed the issue-6 branch 2 times, most recently from de443ef to 72edd4c Compare August 2, 2021 08:40

otaviof force-pushed the issue-6 branch from 72edd4c to a9c5e27 Compare August 2, 2021 08:47

otaviof requested a review from coreydaley August 2, 2021 08:51

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 3, 2021

openshift-ci bot merged commit 3eb6726 into shipwright-io:main Aug 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Target Namespace #10

Target Namespace #10

otaviof commented May 17, 2021 •

edited

Loading

otaviof commented May 21, 2021

gabemontero left a comment

adambkaplan left a comment

adambkaplan Jun 8, 2021

adambkaplan Jun 8, 2021

otaviof Jun 9, 2021

otaviof commented Jul 1, 2021

coreydaley Jul 27, 2021

adambkaplan Jul 29, 2021

coreydaley Jul 27, 2021

otaviof Jul 29, 2021

adambkaplan Jul 29, 2021

adambkaplan left a comment

openshift-ci bot commented Jul 29, 2021

adambkaplan commented Jul 29, 2021

coreydaley Jul 30, 2021

otaviof Aug 2, 2021

coreydaley Jul 30, 2021

otaviof Aug 2, 2021

coreydaley Jul 30, 2021

coreydaley Jul 30, 2021

otaviof Aug 2, 2021

coreydaley commented Aug 3, 2021

Target Namespace #10

Target Namespace #10

Conversation

otaviof commented May 17, 2021 • edited Loading

Changes

Submitter Checklist

Release Notes

otaviof commented May 21, 2021

gabemontero left a 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

Choose a reason for hiding this comment

otaviof commented Jul 1, 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

adambkaplan left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jul 29, 2021

adambkaplan commented Jul 29, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coreydaley commented Aug 3, 2021

otaviof commented May 17, 2021 •

edited

Loading