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

Add tests for new operator-sdk related code #122

Merged

Conversation

jpkrohling
Copy link
Contributor

@jpkrohling jpkrohling commented Nov 21, 2018

Closes #119
Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de

@jpkrohling
Copy link
Contributor Author

This change is Reviewable

@jpkrohling
Copy link
Contributor Author

You'll note that I'm using "strategy" and "strategy chooser" in the test already, even though I haven't moved the old controllers to this new name yet. This is to keep the PR small and focused, and I'll send another PR with the renaming.

@jpkrohling
Copy link
Contributor Author

e2e tests are passing:

ok  	github.com/jaegertracing/jaeger-operator/test/e2e	206.719s

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
@jpkrohling jpkrohling force-pushed the 119-Add-tests-to-new-controller branch from f355798 to 6f8ed3d Compare November 21, 2018 16:16
@codecov
Copy link

codecov bot commented Nov 21, 2018

Codecov Report

Merging #122 into master will increase coverage by 3.51%.
The diff coverage is 25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #122      +/-   ##
==========================================
+ Coverage   92.12%   95.64%   +3.51%     
==========================================
  Files          27       27              
  Lines        1143     1148       +5     
==========================================
+ Hits         1053     1098      +45     
+ Misses         86       38      -48     
- Partials        4       12       +8
Impacted Files Coverage Δ
pkg/controller/jaeger/jaeger_controller.go 54.87% <25%> (+54.87%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c83075...d615a7d. Read the comment docs.

Copy link
Contributor

@objectiser objectiser left a comment

Choose a reason for hiding this comment

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

LGTM - only a couple of comments.

)

type fakeStrategy struct {
dependencies func() []batchv1.Job
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to this PR, but just wondering if dependencies should be strictly typed to Job instead of also runtime.Object - to cover examples that we discussed about setting up configmaps as dependencies? Just raised here as a note - if relevant I will raise as separate issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, I think it should. We talked about routes/config maps being dependencies, but they are usually available instantly. The assumption is then that bigger tasks would be wrapped within a batch job, which provides a "succeed" flag, indicating whether or not the dependency processing finished executing. The batch job could, for instance, deploy another operator and wait for a CR to be ready and available.

pkg/controller/jaeger/jaeger_controller_test.go Outdated Show resolved Hide resolved
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
@jpkrohling
Copy link
Contributor Author

PR updated.

Copy link
Contributor

@objectiser objectiser left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 8 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@jpkrohling jpkrohling merged commit 96ccc24 into jaegertracing:master Nov 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants