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 prependedContainers #51

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ribbybibby
Copy link

@ribbybibby ribbybibby commented Sep 8, 2020

What and why?

Adds a field which allows sidecars to be prepended to the top of the list of the containers. This allows use of this workaround for delaying an application until the sidecars are ready:
https://medium.com/@marko.luksa/delaying-application-start-until-sidecar-is-ready-2ec2d21a7b74

Issue: #49

Testing Steps

Please provide adequate testing steps (including screenshots if necessary).
Include any test fixtures or sample configurations in your commit.

  • 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!

@ribbybibby
Copy link
Author

@byxorna @kirooshu @alex-laties

@komapa
Copy link
Contributor

komapa commented Sep 10, 2020

@ribbybibby, I agree that workaround is good until there is a proper solution. What I would recommend as a patch here though is to have a flag for prepend all or append all (current behavior). I think having the prependContainers is confusing and most of the time you do not really care if there are some other containers between your "sidecar" container and the main container, they can start just as normal if they do not have the postStart logic.

So I would vote here for just having a global flag on the injection config for whether to prepend the containers or append (default) and that should solve your use case, right?

@komapa komapa self-requested a review September 10, 2020 21:53
@ribbybibby
Copy link
Author

Thanks for looking at the PR @kirooshu. Yep, that works for me. I'll make the changes.

This bool modifies the behaviour of the container injection (prepend, append).
@ribbybibby
Copy link
Author

@kirooshu I've pushed the suggested changes.

pkg/server/webhook.go Outdated Show resolved Hide resolved
Copy link
Contributor

@komapa komapa left a comment

Choose a reason for hiding this comment

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

Thank you. LGTM

@ribbybibby
Copy link
Author

🙇 Thanks for the approve @kirooshu. Is this good to be merged?

@ribbybibby
Copy link
Author

Ping - just checking if anyone has had a chance to look at this? @alex-laties @defect

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