Unit test install strategy ALM-227 #132

ecordell · 2017-10-30T13:48:28Z

refactored install strategy bit to separate concerns and make things more testable
added a client interface that the install strategy uses
added tests for install behavior for when things exist in the cluster and when they don't

ericavonb · 2017-10-30T15:08:01Z

apis/clusterserviceversion/v1alpha1/types.go

@@ -130,6 +130,7 @@ const (
 	CSVReasonRequirementsNotMet  ConditionReason = "RequirementsNotMet"
 	CSVReasonRequirementsMet     ConditionReason = "AllRequirementsMet"
 	CSVReasonComponentFailed     ConditionReason = "InstallComponentFailed"
+	CSVReasonInvalidStrategy     ConditionReason = "InstallStrategyInvalid"


InstallStraegyInvalid reads a little weird to me. Can we reverse the order on it, i.e. InvalidInstallStrategy?

ericavonb · 2017-10-30T15:25:07Z

client/deployment_install_client.go

+type InstallStrategyDeploymentInterface interface {
+	CreateRole(role *v1beta1rbac.Role) (*v1beta1rbac.Role, error)
+	CreateRoleBinding(roleBinding *v1beta1rbac.RoleBinding) (*v1beta1rbac.RoleBinding, error)
+	GetOrCreateServiceAccount(serviceAccount *v1.ServiceAccount) (*v1.ServiceAccount, error)


The expected behavior of this function isn't very clear from the name. I think simply making it CreateServiceAccount where it's a no-op if it already exists (or EnsureServiceCountExists?) is fine.

It actually does return the ServiceAccount it finds if it's already there - it's not a no-op if it already exists.

It didn't look like anything other than the name, which is set in alm/install/deployment and by necessity the same.

Should this really be an upsert, i.e. CreateOrUpdateServiceAccount? I think what's throwing me off is maybe the service account will match what you gave the function, maybe it won't (if it already exists).

ericavonb · 2017-10-30T15:28:25Z

client/deployment_install_client.go

+	CreateRoleBinding(roleBinding *v1beta1rbac.RoleBinding) (*v1beta1rbac.RoleBinding, error)
+	GetOrCreateServiceAccount(serviceAccount *v1.ServiceAccount) (*v1.ServiceAccount, error)
+	CreateDeployment(deployment *v1beta1extensions.Deployment) (*v1beta1extensions.Deployment, error)
+	CheckServiceAccount(serviceAccountName string) (bool, error)


Check what? It might be simpler to just have this be the simple GetServiceAccountByName, otherwise maybe CheckServiceAccountExists or something else to clarify what this means.

ericavonb · 2017-10-30T15:36:44Z

client/deployment_install_client.go

+	GetOrCreateServiceAccount(serviceAccount *v1.ServiceAccount) (*v1.ServiceAccount, error)
+	CreateDeployment(deployment *v1beta1extensions.Deployment) (*v1beta1extensions.Deployment, error)
+	CheckServiceAccount(serviceAccountName string) (bool, error)
+	CheckOwnedDeployments(owner metav1.ObjectMeta, deploymentSpecs []v1beta1extensions.DeploymentSpec) (bool, error)


Same here. This function weirdly mixes client and consumer concerns. Doing GetOwnedDeployments and having the compare logic in install/deployment.go makes it much easier to understand what's going on.

ericavonb · 2017-10-30T15:43:27Z

client/deployment_install_client.go

+
+type InstallStrategyDeploymentClient struct {
+	opClient  opClient.Interface
+	Namespace string


I'm unclear on how this namespace is used. Are all actions scoped to this namespace? As a consumer of this client, it's very opaque. Either consistently use Namespace and rename this like InstallStrategyDeploymentClientForNamespace or whatever, or (my preference) add namespace string as an argument to each function in the interface that needs it.

Yeah, all actions are restricted to the namespace. I can make that clear with the name

ericavonb · 2017-10-30T15:46:08Z

client/deployment_install_client.go

+func (c *InstallStrategyDeploymentClient) CheckOwnedDeployments(owner metav1.ObjectMeta, deploymentSpecs []v1beta1extensions.DeploymentSpec) (bool, error) {
+	existingDeployments, err := c.opClient.ListDeploymentsWithLabels(c.Namespace, map[string]string{
+		"alm-owner-name":      owner.Name,
+		"alm-owner-namespace": owner.Namespace,


Personal pref, I would prefer to have CheckOwnedDeployments(ownerName, ownerNamespace...) or whatever to clarify what it's matching on.

We might change this query to user ownerreferences in the future, so I think it makes sense to keep the whole owner

I think I have some fear still from experiences where parts of the codebase break unexpectedly when you change something about the object because it relied on some obscure field on the object you didn't even know about.

I'll trust your judgement on where we need flexibility more than guaranteeing future stability. 👍

ericavonb · 2017-10-30T16:34:50Z

install/deployment.go

+
+func NewStrategyDeploymentInstaller(opClient opClient.Interface, ownerMeta metav1.ObjectMeta, ownerType metav1.TypeMeta) StrategyInstaller {
+	return &StrategyDeploymentInstaller{
+		strategyClient: client.NewInstallStrategyDeploymentClient(opClient, ownerMeta.Namespace),


It's a little awkward, but better separation of concerns if the consumer instantiates and passes along the alm/client instead of opClient; means we can remove operator-client from the entire alm/install package and cleanly pass the client from the ALM operator's instantiation (create the alm/client somewhere after here: https://github.com/ecordell/alm/blob/eb2937fb06723562be5d077b860b70a8368ce367/queueinformer/queueinformer_operator.go#L22)

Moved it up a level but it's hard to move it much higher than the strategy resolver because each install strategy will need a different client:

func (r *StrategyResolver) InstallerForStrategy(strategyName string, opClient opClient.Interface, ownerMeta metav1.ObjectMeta) StrategyInstaller { switch strategyName { case InstallStrategyNameDeployment: strategyClient := client.NewInstallStrategyDeploymentClient(opClient, ownerMeta.Namespace) return NewStrategyDeploymentInstaller(strategyClient, ownerMeta) } //... }

When we get to the point of having more than one install strategy type we can revisit refactoring

ericavonb · 2017-10-30T16:38:22Z

install/deployment.go

+type StrategyDeploymentInstaller struct {
+	strategyClient client.InstallStrategyDeploymentInterface
+	ownerMeta      metav1.ObjectMeta
+	ownerType      metav1.TypeMeta


I know this isn't from commits in this pr but it looks like ownerType is never used.

ericavonb · 2017-10-31T17:18:03Z

client/deployment_install_client.go

+	}
+	if err == nil {
+		return foundAccount, nil
+	}


It might be clearer to do,

if err == nil { return foundAccount, nil } if !apierrors.IsNotFound(err) { return nil, errors.Wrap(err, "checking for existing serviceacccount failed") }

ericavonb · 2017-10-31T17:40:41Z

client/deployment_install_client.go

+	if err != nil && !apierrors.IsAlreadyExists(err) {
+		return createdAccount, errors.Wrap(err, "creating serviceacccount failed")
+	}
+	return createdAccount, nil


Do you need to re-fetch the serviceaccount if you get a IsAlreadyExists error?

Nope, we only need the name to create the rolebinding

But won't createdAccount be nil? You could return serviceAccount but I still find it unclear about what you're getting back. In the first Get, the return value foundAccount != serviceAccount necessarily. And if the Create is successful, the createdAccount will have additional data on it that serviceAccount does not. I'd rather have consistent behavior, which might mean updating the service account if found.

It will be nil, but that's the correct value for it for this return. I've changed it explicitly to nil so that it's clear.

ericavonb · 2017-10-31T17:42:04Z

install/deployment.go

+
+	// Check deployments
+	if found, err := i.checkForOwnedDeployments(i.ownerMeta, strategy.DeploymentSpecs); !found {
+		log.Infof("deployments not found")


log.Errorf?

Changed to debugf, which fits a bit better I think

ericavonb · 2017-11-01T14:51:08Z

client/deployment_install_client.go

+	if err != nil && !apierrors.IsAlreadyExists(err) {
+		return createdAccount, errors.Wrap(err, "creating serviceacccount failed")
+	}
+	return createdAccount, nil


But won't createdAccount be nil? You could return serviceAccount but I still find it unclear about what you're getting back. In the first Get, the return value foundAccount != serviceAccount necessarily. And if the Create is successful, the createdAccount will have additional data on it that serviceAccount does not. I'd rather have consistent behavior, which might mean updating the service account if found.

…tall-strategy Unit test install strategy ALM-227

ecordell added 2 commits October 30, 2017 09:46

Refactor install strategies

61d5e91

Add tests for deployment install strategy with role generation

c24854b

ecordell force-pushed the unit-test-install-strategy branch from c856695 to c24854b Compare October 30, 2017 13:55

ecordell requested review from jzelinskie and ericavonb October 30, 2017 13:56

ecordell changed the title ~~Unit test install strategy~~ Unit test install strategy ALM-227 Oct 30, 2017

Deploy install strategy: Add tests for error cases

eb2937f

ecordell force-pushed the unit-test-install-strategy branch from ed34e85 to eb2937f Compare October 30, 2017 14:47

ecordell removed the request for review from jzelinskie October 30, 2017 15:31

ericavonb suggested changes Oct 30, 2017

View reviewed changes

ecordell force-pushed the unit-test-install-strategy branch from 7817dc8 to 2bc5973 Compare October 30, 2017 19:36

ericavonb previously approved these changes Oct 30, 2017

View reviewed changes

ecordell dismissed ericavonb’s stale review via aa1eba7 October 31, 2017 11:54

ecordell force-pushed the unit-test-install-strategy branch 3 times, most recently from a7ae91a to 86117dd Compare October 31, 2017 12:16

ericavonb previously approved these changes Oct 31, 2017

View reviewed changes

ecordell dismissed ericavonb’s stale review via 4ecd4c7 October 31, 2017 21:49

ecordell force-pushed the unit-test-install-strategy branch 2 times, most recently from 4ecd4c7 to 1bbd0ec Compare October 31, 2017 21:50

ericavonb suggested changes Nov 1, 2017

View reviewed changes

ecordell force-pushed the unit-test-install-strategy branch from 1bbd0ec to 5fc7538 Compare November 1, 2017 15:12

Refactor deployment strategy client + naming fixes

65a0c84

ecordell force-pushed the unit-test-install-strategy branch from 5fc7538 to 65a0c84 Compare November 1, 2017 15:55

ericavonb approved these changes Nov 1, 2017

View reviewed changes

ecordell merged commit 3365990 into operator-framework:master Nov 1, 2017

jzelinskie deleted the unit-test-install-strategy branch November 6, 2017 16:33

njhale pushed a commit to njhale/operator-lifecycle-manager that referenced this pull request Sep 10, 2018

Merge pull request operator-framework#132 from ecordell/unit-test-ins…

4a95b86

…tall-strategy Unit test install strategy ALM-227

ecordell added a commit to ecordell/operator-lifecycle-manager that referenced this pull request Mar 8, 2019

Merge pull request operator-framework#132 from ecordell/unit-test-ins…

8a2a773

…tall-strategy Unit test install strategy ALM-227

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unit test install strategy ALM-227 #132

Unit test install strategy ALM-227 #132

ecordell commented Oct 30, 2017

ericavonb Oct 30, 2017

ericavonb Oct 30, 2017

ecordell Oct 30, 2017

ericavonb Oct 30, 2017

ericavonb Oct 30, 2017

ericavonb Oct 30, 2017

ericavonb Oct 30, 2017

ecordell Oct 30, 2017

ericavonb Oct 30, 2017

ecordell Oct 30, 2017

ericavonb Oct 30, 2017

ericavonb Oct 30, 2017

ecordell Oct 30, 2017

ericavonb Oct 30, 2017

ericavonb Oct 31, 2017

ericavonb Oct 31, 2017

ecordell Oct 31, 2017

ericavonb Nov 1, 2017

ecordell Nov 1, 2017

ericavonb Oct 31, 2017

ecordell Oct 31, 2017

ericavonb Nov 1, 2017

Unit test install strategy ALM-227 #132

Unit test install strategy ALM-227 #132

Conversation

ecordell commented Oct 30, 2017

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment