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

Create missing CA config maps on deployment controller #1347

Conversation

jpkrohling
Copy link
Contributor

This PR changes the deployment controller so that it always attempt to create the configmaps in the namespace for the target deployments, as long as sidecar injection is desired for the deployment.

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

@mergify mergify bot requested a review from rubenvp8510 December 15, 2020 14:22
@jpkrohling jpkrohling force-pushed the jpkrohling/create-missing-ca-configmaps branch 2 times, most recently from bfb88bf to 5db2056 Compare December 15, 2020 14:37
@codecov
Copy link

codecov bot commented Dec 15, 2020

Codecov Report

Merging #1347 (8c5f86f) into master (bf1bc27) will increase coverage by 0.20%.
The diff coverage is 42.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1347      +/-   ##
==========================================
+ Coverage   86.02%   86.23%   +0.20%     
==========================================
  Files          90       90              
  Lines        5125     5143      +18     
==========================================
+ Hits         4409     4435      +26     
+ Misses        548      538      -10     
- Partials      168      170       +2     
Impacted Files Coverage Δ
pkg/controller/deployment/deployment_controller.go 33.62% <31.14%> (+14.57%) ⬆️
pkg/inject/sidecar.go 94.93% <100.00%> (+0.16%) ⬆️

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 bf1bc27...8c5f86f. Read the comment docs.

@rubenvp8510
Copy link
Collaborator

LGTM

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.

Only one minor comment.

pkg/controller/deployment/deployment_controller.go Outdated Show resolved Hide resolved
@jpkrohling jpkrohling force-pushed the jpkrohling/create-missing-ca-configmaps branch from 5db2056 to a891a1d Compare December 15, 2020 15:08
Copy link
Contributor

@kevinearls kevinearls left a comment

Choose a reason for hiding this comment

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

LGTM

objectiser
objectiser previously approved these changes Dec 15, 2020
rubenvp8510
rubenvp8510 previously approved these changes Dec 15, 2020
@mergify mergify bot dismissed stale reviews from objectiser and rubenvp8510 December 15, 2020 16:35

Pull request has been modified.

@rubenvp8510
Copy link
Collaborator

Why the change from patch to update?

@@ -108,74 +109,61 @@ func (r *ReconcileDeployment) Reconcile(request reconcile.Request) (reconcile.Re
err = r.rClient.Get(ctx, types.NamespacedName{Name: request.Namespace}, ns)
Copy link
Contributor

Choose a reason for hiding this comment

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

@jpkrohling @rubenvp8510 What is the difference between the r.rClient and r.client? Seems both are used in this class?

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
@jpkrohling jpkrohling force-pushed the jpkrohling/create-missing-ca-configmaps branch from a408d7a to 8c5f86f Compare December 15, 2020 20:09
@objectiser objectiser merged commit 010535a into jaegertracing:master Dec 17, 2020
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.

4 participants