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/inject init containers #8

Merged
merged 9 commits into from
Mar 11, 2019

Conversation

iwilltry42
Copy link
Contributor

@iwilltry42 iwilltry42 commented Mar 7, 2019

What and why?

Note: this PR assumes that the hostAliases PR got merged already (too lazy for some more merges).

Note 2: I "simplified" adding tests in internal/pkg/config/config_test.go similar to how it's done in loader_test.go.

Functionality to add initContainers, because they're quite useful sometimes and some functionality, that we currently execute in sidecar containers may only be needed once at the start of a pod.

This works similar to the injection of "normal" containers, meaning that, if there's a name conflict between injection config and the requesting podSpec, the server will return a Duplicate Value error.

Testing Steps

Tested with debug-pod that requests an injection with one initContainer in it.
Configs of debug-pod tested:

  • 0 existing initContainers
  • 1 existing initContainer
  • 1 existing initContainer with name conflict

Same unittests as usual.

Reviewers

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

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.

This looks great @iwilltry42 ! @zach-dunton-sf just opened up #9 to do the same, and there are useful features (fixes to annotation escaping in patches, and unit test refactors) in both PRs that I feel should be merged. Mind coordinating to negotiate which gets merged in?

internal/pkg/config/config_test.go Show resolved Hide resolved
@iwilltry42
Copy link
Contributor Author

@zach-dunton-sf can I merge your changes regarding annotations into this PR? That way we can have all the functionality without having to mess with merge conflicts. And as far as I can see, our changes regarding the injection of initContainers are fairly similar.

@iwilltry42
Copy link
Contributor Author

@byxorna I pasted @zach-dunton-sf 's changes regarding annotations into my branch 👍

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 👍
cc @komapa @defect for final eyes?

Closes #7 and #9

docs/sidecar-configuration-format.md Outdated Show resolved Hide resolved
test/fixtures/k8s/object7.yaml Outdated Show resolved Hide resolved
@byxorna
Copy link
Contributor

byxorna commented Mar 11, 2019

@iwilltry42 ok, thanks for all this! I am merging and will cut a new release in a bit; you can use the :latest image for now, until I write up the release notes :)

@byxorna byxorna merged commit e12e5e6 into tumblr:master Mar 11, 2019
@iwilltry42 iwilltry42 deleted the feature/inject-init-containers branch March 11, 2019 14:12
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

3 participants