Skip to content
This repository has been archived by the owner on Dec 18, 2019. It is now read-only.

e2e test for MSS operator #174

Merged
merged 2 commits into from Sep 27, 2019
Merged

e2e test for MSS operator #174

merged 2 commits into from Sep 27, 2019

Conversation

STEPHINRACHEL
Copy link

@STEPHINRACHEL STEPHINRACHEL commented Sep 20, 2019

Motivation

https://issues.jboss.org/browse/AEROGEAR-9798

What

Added e2e test for MSS operator

How

Using Operator-SDK's test framework

Verification Steps

Run the following from repository

 operator-sdk test local ./test/e2e --go-test-flags  "-v" --namespace mobile-security-service

Checklist:

  • Code has been tested locally by PR requester
  • Changes have been successfully verified by another team member

Progress

  • Finished task
  • TODO

@STEPHINRACHEL STEPHINRACHEL changed the title E2e test mss e2e test for MSS operator Sep 20, 2019
Copy link

@psturc psturc left a comment

Choose a reason for hiding this comment

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

LTGM, but in order to test this in CI, Jenkinsfile needs to be added.
See other operators for a reference, e.g. mdc-operator

@psturc
Copy link

psturc commented Sep 20, 2019

Also, renaming the role*.yaml files would break Makefile commands, for example here. So you can either:

  1. Fix Makefile
  2. Rename role*.yaml files back and use these parameters for running the tests in Jenkins instead. These parameters ensure that all manifest files (role, role_binding, service_account, etc.) and global manifest files (*_crd) get merged and used in test. So it's no longer a problem to have these files named differently. This is how it works behind the scene

@psturc
Copy link

psturc commented Sep 23, 2019

Build is failing because this operator requires a separate CR for deploying a DB for some reason..
https://github.com/aerogear/mobile-security-service-operator/blob/master/Makefile#L46

Also, I'm not sure if we should suddenly add all of the content from vendor folder, which is more than 7k files - @grdryn, wdyt? Should we leave it like that or "unify" it with our other operators, i.e. commit the vendor files?
If we don't decide to commit the vendor folder, there's an additional step we need to add to the Jenkins pipeline which is required to run before running the tests, to ensure that all required dependencies are downloaded for the operator.

@laurafitzgerald
Copy link
Contributor

laurafitzgerald commented Sep 24, 2019

@psturc @STEPHINRACHEL we decided a while back to not include the vendor folder in this repo. You can see comments about it in thsi pr: #87 Although we have included it for other repos and it's not obvious from the linked pr what the internal discussion was and how we resulted in removing it.

@grdryn
Copy link
Member

grdryn commented Sep 25, 2019

I don't have a problem with committing the vendor directory; but if it's being done, consider adding the unused-packages prune option like I've done here: https://github.com/aerogear/unifiedpush-operator/pull/72/files#diff-836546cc53507f6b2d581088903b1785R59

That should drastically reduce the number of files committed. The only thing to think of is that when importing a new package (i.e. one that we haven't used before) from an existing dependency, we'd need to run dep ensure again to get it pulled in.

Copy link

@psturc psturc left a comment

Choose a reason for hiding this comment

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

As @grdryn said above, if we want to add vendor directory, we should at least add it with prune option to remove unused packages.

test/e2e/mobile_security_service_test.go Outdated Show resolved Hide resolved
test/e2e/mobile_security_service_test.go Outdated Show resolved Hide resolved
@STEPHINRACHEL STEPHINRACHEL merged commit a2ca36e into aerogear:master Sep 27, 2019
@STEPHINRACHEL STEPHINRACHEL deleted the e2e-test-mss branch September 27, 2019 10:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants