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

[Feature] Add initContainer injector #9

Closed

Conversation

zach-dunton-sf
Copy link

What and why?

I added support for injecting init containers. We are using it for injecting tools like New Relic in to our applications

There is also a small fix in the updateAnnotations function to escape '/' in annotation names so that the json patch works without overwriting existing pod annotations.

Testing Steps

  • Added unit tests for this feature (make test)

Reviewers

Required reviewers: @byxorna
Request reviews from other people you want to review this PR in the "Reviewers" section on the right.

⚠️ this PR must have at least 2 thumbs from the MAINTAINERS.md of the project before merging!

@yahoocla
Copy link

yahoocla commented Mar 7, 2019

Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄

Copy link
Contributor

@byxorna byxorna left a comment

Choose a reason for hiding this comment

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

awesome PR, thank you for this great PR, @zach-dunton-sf ! I would love to merge both #9 and #8, if we can integrate the features of both (annotation escaping, unit test refactors, etc)

@@ -325,19 +325,19 @@ func mergeVolumeMounts(volumeMounts []corev1.VolumeMount, containers []corev1.Co

func updateAnnotations(target map[string]string, added map[string]string) (patch []patchOperation) {
for key, value := range added {
keyEscaped := strings.Replace(key, "/", "~1", -1)
Copy link
Contributor

Choose a reason for hiding this comment

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

great catch! is this something you could add a unit test for, to assert this behavior is present?

@iwilltry42
Copy link
Contributor

Hi @zach-dunton-sf , would you mind if I integrate your changes regarding the annotations?
Our initContainer injection implementations look fairly similar, so that shouldn't be a problem.

@zach-dunton-sf
Copy link
Author

I wouldn't mind at all.

iwilltry42 pushed a commit to iwilltry42/k8s-sidecar-injector that referenced this pull request Mar 8, 2019
@@ -0,0 +1,4 @@
name: object2
Copy link

Choose a reason for hiding this comment

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

Shouldnt this be object6 ?

if c.Name != expectedName {
t.Fatalf("expected %s Name loaded from %s but got %s", expectedName, cfg, c.Name)
}
if len(c.Environment) != nExpectedEnvironmentVars {
Copy link

Choose a reason for hiding this comment

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

Consider writing an expect function so we dont repeat the same if statement every time. expect(t, c.Environment, "envVars").hasSize(nExpectedEnvironmentVars) would be more readable.

@byxorna
Copy link
Contributor

byxorna commented Mar 11, 2019

Thanks for the contribution @zach-dunton-sf! Changes are in master, and you can use the docker image tagged :latest shortly. I will cut a new release shortly :)

Supplanted by #8

@byxorna byxorna closed this Mar 11, 2019
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.

None yet

5 participants