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.

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

Tests/modes #88

Merged
merged 7 commits into from
Mar 29, 2018
Merged

Tests/modes #88

merged 7 commits into from
Mar 29, 2018

Conversation

ObiWahn
Copy link
Contributor

@ObiWahn ObiWahn commented Mar 28, 2018

Test if deployments work in development and production mode

@ObiWahn ObiWahn force-pushed the tests/modes branch 3 times, most recently from 1fd689d to 5db8006 Compare March 28, 2018 12:23
Copy link
Contributor

@ewoutp ewoutp left a comment

Choose a reason for hiding this comment

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

  1. Couple of go issues.
  2. Tweak count values of the specification (before creating deployment)

// check environment
longOrSkip(t)

// FIXME - add code
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO?

@@ -0,0 +1,108 @@
//
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused by the filename.
This is about environments, not modes.

subTest(t, api.DeploymentModeCluster, api.StorageEngineRocksDB)
}

func subTest(t *testing.T, mode api.DeploymentMode, engine api.StorageEngine) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use a more descriptive function name
subTest can apply to any test

Copy link
Contributor

Choose a reason for hiding this comment

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

and document it

deploymentTemplate.Spec.Environment = api.NewEnvironment(api.EnvironmentProduction)
deploymentTemplate.Spec.SetDefaults(deploymentTemplate.GetName()) // this must be last

var dbserverCount int = *deploymentTemplate.Spec.DBServers.Count
Copy link
Contributor

Choose a reason for hiding this comment

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

go: dbserverCount := *deploymentTemplate.Spec.DBServers.Count


numNodes := len((*nodeList).Items)

var failExpected bool = false
Copy link
Contributor

Choose a reason for hiding this comment

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

go: failExpected := false

t.Fatalf("Unable to receive node list: %v", err)
}

numNodes := len((*nodeList).Items)
Copy link
Contributor

Choose a reason for hiding this comment

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

go: numNodes := len(nodeList.Items)
. will de-reference on its own.

}

// Create deployment
deployment, err := deploymentClient.DatabaseV1alpha().ArangoDeployments(k8sNameSpace).Create(deploymentTemplate)
Copy link
Contributor

Choose a reason for hiding this comment

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

deployment is never used, so you can replace this with

if _, err := deploymentClient.DatabaseV1alpha().ArangoDeployments(k8sNameSpace).Create(deploymentTemplate); err != nil { ...

t.Fatalf("Create deployment failed: %v", err)
}

deployment, err = waitUntilDeployment(deploymentClient, deploymentTemplate.GetName(), k8sNameSpace, deploymentIsReady())
Copy link
Contributor

Choose a reason for hiding this comment

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

After above change, this will be the first use of deployment, so deployment, err := ...

deploymentTemplate.Spec.StorageEngine = api.NewStorageEngine(engine)
deploymentTemplate.Spec.TLS = api.TLSSpec{} // should auto-generate cert
deploymentTemplate.Spec.Environment = api.NewEnvironment(api.EnvironmentProduction)
deploymentTemplate.Spec.SetDefaults(deploymentTemplate.GetName()) // this must be last
Copy link
Contributor

Choose a reason for hiding this comment

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

I expected the spec to set Count values above the number of nodes.
That way we can also test what happens when you have a 3 node cluster with 4 dbservers/agents/coordinators.

deploymentTemplate.Spec.Mode = api.NewMode(mode)
deploymentTemplate.Spec.StorageEngine = api.NewStorageEngine(engine)
deploymentTemplate.Spec.TLS = api.TLSSpec{} // should auto-generate cert
deploymentTemplate.Spec.Environment = api.NewEnvironment(api.EnvironmentProduction)
Copy link
Contributor

Choose a reason for hiding this comment

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

You must specify an Image in production mode.

Added validation of spec to ensure that is all good.

[ci LONG=1]
[ci TESTOPTIONS="-test.run ^TestProduction$"]
@ewoutp
Copy link
Contributor

ewoutp commented Mar 29, 2018

Couple of go issues remain, but let's merge it now.

[ci LONG=1]
[ci TESTOPTIONS="-test.run ^TestProduction$"]
@ObiWahn ObiWahn merged commit 5cf25ee into master Mar 29, 2018
@ObiWahn ObiWahn deleted the tests/modes branch March 29, 2018 12:26
@ghost ghost mentioned this pull request Apr 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants