-
Notifications
You must be signed in to change notification settings - Fork 569
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] helm: add service.extraPorts to all components #10659
Conversation
Signed-off-by: TheRealNoob <[email protected]>
Signed-off-by: TheRealNoob <[email protected]>
Signed-off-by: TheRealNoob <[email protected]>
Signed-off-by: TheRealNoob <[email protected]>
Thank you for working on the PR. Before moving the changes further, could you provide more details about the problem you were solving
Is this a theoretical case? Or can you share a practical scenario where ports from mimir components were missing and needed to be exposed to the cluster? |
@narqo Thank you for having a look. The use-case that I'm working on is exposing prometheus and alertmanager to the internet behind an oauth2 sidecar. I'm setting them up under new ingress objects so as to not interfere with the main ingress which I have using basicAuth authentication. I saw in the chart that there are already comments in place on the Let me know if you need me to go into any more detail. In short, if someone adds an extraContainer that exposes a new port, there is a very high likelihood that they'll need to expose it on the SVC as well. |
I think the lint check is a false positive caused by the single quote in the word |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delayed review. The changes look good to me, I believe the markdown-lint's failure is indeed a false-positive. I can take care of it.
@TheRealNoob the SLA signing job says
It seems that this comes after one of your commits that uses this author's name and the email. Could you either add the email to your github account, or update the changeset to use the git author, that signs the SLA. Sorry for extra bureaucracy. |
Head branch was pushed to by a user without write access
9ecb4aa
to
84f8027
Compare
Signed-off-by: TheRealNoob <[email protected]>
Signed-off-by: TheRealNoob <[email protected]>
Signed-off-by: Vladimir Varankin <[email protected]>
Signed-off-by: TheRealNoob <[email protected]>
This reverts commit 332b721.
84f8027
to
0ef547c
Compare
well that was fun. I was previously changing some settings on my gitconfig to make it easier to work with two accounts from one machine (work and personal) which led to mis-signed commits. And then the marvelous joys of learning rebase (again). Never fun, but hopefully one day I'll be able to do something without having to look it up :). Apologies for the troubles. |
Signed-off-by: TheRealNoob <[email protected]>
What this PR does
In the Helm chart, add
service.extraPorts
to all components that allow definingextraContainers
. The intention being that people may expose additional ports on the Pod, and that should be exposed from the Service as well.I haven't bumped the chart version, could you tell me what it should be bumped to? I'm not familiar with the format being used.
Which issue(s) this PR fixes or relates to
Didn't open one
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]
.about-versioning.md
updated with experimental features.