Skip to content
This repository has been archived by the owner on Nov 15, 2022. It is now read-only.

Adding OpenShift tests #465

Merged
merged 1 commit into from
Jan 11, 2018
Merged

Conversation

ashetty1
Copy link
Collaborator

@ashetty1 ashetty1 commented Nov 20, 2017

For OpenShift, we reuse the functions which we use for k8s tests. So for this purpose, this commit puts those functions into a separate file: tests/e2e/e2e.go

The k8s tests are put under: tests/e2e/e2e_k8s_test.go

The OpenShift tests are in tests/e2e/e2e_os_test.go

The e2e script has been modified to run only k8s tests by default (by using the go test -run k8s)

To run OpenShift tests: go test -run os

Copy link
Member

@surajssd surajssd left a comment

Choose a reason for hiding this comment

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

Why copy code when we can reuse it?

@ashetty1
Copy link
Collaborator Author

@surajssd OpenShift functions are a bit different with arguments passed, and of course, oc commands. We either modify the k8s helper functions to accommodate oc or maintain two different files. I went with the latter because the former option might constrain us in the future.

@concaf
Copy link
Collaborator

concaf commented Nov 21, 2017

@ashetty1 could you link to the code where we copied this from?

@cdrage
Copy link
Collaborator

cdrage commented Nov 21, 2017

IMO I agree with @surajssd let's modify the current code we have to use both Kubernetes / OpenShift, or even a combination of both (have a flag to say if it's an OpenShift test?) having to separate code-bases will be a hassle.

@ashetty1
Copy link
Collaborator Author

@cdrage sure. Sounds good.

@ashetty1
Copy link
Collaborator Author

@surajssd @cdrage what we could do is use template queries for both kubectl and oc; then it would just be a matter of using one of the binaries while running the cli. This would mean we would move away from k8s go api. Are you okay with that?

@surajssd
Copy link
Member

surajssd commented Nov 22, 2017

@cdrage @ashetty1

so what I think is first let's run our entire test suite that runs on semaphore on openshift(can be started using oc cluster up), that way we ensure that we have moved from k8s to openshift without problems.

@cdrage I had a chat with @ashetty1 where we sorted out how we can reuse the functions. So all the operations which are generic like checking if pod is up or endpoint is responding can use the client-go based already defined functions but things that are openshift specific like checking out if buildconfig is created or imagestream is created can have separate functions.

So

@cdrage
Copy link
Collaborator

cdrage commented Nov 22, 2017

@surajssd I agree.

I do suggest that we try and modular it as much as possible (no hardcoded variables specific to OpenShift...) so we can possible push this into a separate repo / to other projects (such as Kompose and other Kubernetes projects).

@ashetty1 ashetty1 changed the title [WIP] Adding OpenShift tests Adding OpenShift tests Nov 28, 2017
@cdrage
Copy link
Collaborator

cdrage commented Nov 29, 2017

Saw your changes, they LGTM to me so far. Are you ready for us to do another review @ashetty1 ?

@ashetty1
Copy link
Collaborator Author

ashetty1 commented Dec 5, 2017

@cdrage yes, please. I am working on adding more tests but that won't be affected.

@ashetty1 ashetty1 force-pushed the openshift_test branch 2 times, most recently from 9237042 to ca862b7 Compare December 7, 2017 10:29
Copy link
Collaborator

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

Other than 2 small comments, the code LGTM. Does this pass all-green for you dev side? (to be honest, SemaphoreCI is a bit broken right now...)

// Interval of 5
return wait.Poll(5, jobTimeout, func() (bool, error) {
var buildOut bool
buildStatus, err := runCmd("oc get build --namespace=" + namespace + " --template='{{ range .items }}{{ if (eq .metadata.name \"" +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add a TODO to this function indicating that this will be eventually replaced by an API? Not too sure.. Maybe we could keep it as using runCmd.


func waitForBuildComplete(namespace string, buildName string) error {
// Interval of 5
return wait.Poll(5, jobTimeout, func() (bool, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

make this a global variable so we can change it in the future?

@ashetty1 ashetty1 force-pushed the openshift_test branch 2 times, most recently from 4fcccef to 18f2430 Compare December 13, 2017 10:19
@ashetty1
Copy link
Collaborator Author

@cdrage tests passed locally. Have made the changes you requested for. PTAL.

@cdrage
Copy link
Collaborator

cdrage commented Dec 13, 2017

This LGTM. @surajssd can you do a quick-review and we can merge?

@ashetty1
Copy link
Collaborator Author

@surajssd hold on. Just need to change one line before you review.

@ashetty1
Copy link
Collaborator Author

@surajssd have pushed all the changes.

@ashetty1
Copy link
Collaborator Author

ashetty1 commented Jan 2, 2018

ping @kadel @surajssd, could any of you quickly review this and give this a go.

@cdrage
Copy link
Collaborator

cdrage commented Jan 2, 2018

This may be a lot more work than intended, but I'm converting all the e2e test examples to redis / guestbook-go instead, see here: #554

This code LGTM, but of course, the tests are not. Perhaps wait until I'm done with #554 and you can copy and paste the changes to the OpenShift tests? Does that sound good? @ashetty1

@ashetty1
Copy link
Collaborator Author

ashetty1 commented Jan 9, 2018

Rebased and added s2i tests too.

@ashetty1
Copy link
Collaborator Author

Cc: @cdrage @surajssd

@cdrage
Copy link
Collaborator

cdrage commented Jan 10, 2018

@ashetty1 Thank you! I've tested this locally and it works against minishift.

Are you able to add the VERBOSE and PARALLEL commands similar to: https://github.com/ashetty1/kedge/blob/133f0ed195312caf6092c5719ad28573bf334425/Makefile#L75 so we can run this on Semaphore / CI with verbosity?

For OpenShift, we reuse the functions which we use for \
k8s tests. So for this purpose, this commit puts those \
functions into a separate file: tests/e2e/e2e.go

The k8s tests are put under: tests/e2e/e2e_k8s_test.go

The OpenShift tests are in tests/e2e/e2e_os_test.go

The e2e script has been modified to run only k8s tests \
by default (by using the `go test -run k8s`)

To run OpenShift tests: `make test-e2e-os` which runs \
`go test -run os`
@ashetty1
Copy link
Collaborator Author

@cdrage fixed that: make test-e2e-os gets redirected to make test-e2e.

@cdrage
Copy link
Collaborator

cdrage commented Jan 11, 2018

Tested this locally. This LGTM. Let's get this merged in so we can test it on SemaphoreCI.

@cdrage
Copy link
Collaborator

cdrage commented Jan 11, 2018

Thanks so much!

@cdrage cdrage merged commit caf3053 into kedgeproject:master Jan 11, 2018
@kadel
Copy link
Member

kadel commented Jan 11, 2018

it looks like it doesn't work on SemaphoreCI

   Image pull complete
-- Checking Docker daemon configuration ... FAIL
   Error: did not detect an --insecure-registry argument on the Docker daemon
   Solution:

     Ensure that the Docker daemon is running with the following argument:
     	--insecure-registry 172.30.0.0/16

https://semaphoreci.com/cdrage/kedge/branches/master/builds/222

@ashetty1
Copy link
Collaborator Author

@kadel @cdrage To run oc cluster up, we need to add that to the docker config: https://github.com/openshift/origin/blob/master/docs/cluster_up_down.md#getting-started

@cdrage
Copy link
Collaborator

cdrage commented Jan 11, 2018

@kadel I'm on it, haha. Been troubleshooting. I'm using minishift now on SemaphoreCI.

@kadel
Copy link
Member

kadel commented Jan 15, 2018

Still failing :-( It is false PASS :-(

kadel pushed a commit to kadel/kedge that referenced this pull request Jan 15, 2018
For OpenShift, we reuse the functions which we use for \
k8s tests. So for this purpose, this commit puts those \
functions into a separate file: tests/e2e/e2e.go

The k8s tests are put under: tests/e2e/e2e_k8s_test.go

The OpenShift tests are in tests/e2e/e2e_os_test.go

The e2e script has been modified to run only k8s tests \
by default (by using the `go test -run k8s`)

To run OpenShift tests: `make test-e2e-os` which runs \
`go test -run os`
@cdrage
Copy link
Collaborator

cdrage commented Jan 15, 2018

@kadel Issue has been opened here: #568 (comment) to troubleshoot this week what's happening.

kadel pushed a commit to kadel/kedge that referenced this pull request Jan 17, 2018
For OpenShift, we reuse the functions which we use for \
k8s tests. So for this purpose, this commit puts those \
functions into a separate file: tests/e2e/e2e.go

The k8s tests are put under: tests/e2e/e2e_k8s_test.go

The OpenShift tests are in tests/e2e/e2e_os_test.go

The e2e script has been modified to run only k8s tests \
by default (by using the `go test -run k8s`)

To run OpenShift tests: `make test-e2e-os` which runs \
`go test -run os`
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants