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

Fixes #751 - A pod-culler bug #752

Merged
merged 3 commits into from
Jul 8, 2018

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Jul 6, 2018

@betatim @minrk Sorry for causing this, this closes #751.

the helper jupyterhub.matchLabels will return labels based on the files parent directory (component: pod-culler), but that can be overridden if one passes the helper a scope containing a .componentLabel.

Note that the output will be:
component=singleuser-server,app=jupyterhub,release=prod
Instead of:
component=pod-culler,app=jupyterhub,release=prod

Using the app and release as selectors as well is done systematically on all pods according to helm chart best practices.

@consideRatio consideRatio force-pushed the fix-pod-culler-selector-pr branch from d3b1266 to e457c06 Compare July 6, 2018 18:35
access to the current scope containing .Release.Name, .Values.rbac.enabled and
similar values.

{{ include "jupyterhub.commonLabels" . }}
Copy link
Member

Choose a reason for hiding this comment

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

This is an example of calling the helper called jupyterhub.commonLabels with scope .?


{{ include "demo.bananaPancakes" (dict "pancakes" 5 "bananas" 3) }}

To let a helper acces the current scope along with additional values we can
Copy link
Member

Choose a reason for hiding this comment

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

acces -> access


NOTE:
The ordering of merge is crucial, the latter argument is copied and merged
into the former. So if you would swap the order you would influence the
Copy link
Member

Choose a reason for hiding this comment

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

A different way of saying this would be that an argument to the right overwrites an argument to the left of it? In this case if appLabel was also defined in the current scope we'd end up with the value from the current scope being available to the helper?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I think so, but this will never be an issue I think, because I don't know how to add things to the current scope without doing it the other way around:

{{- $_ := merge . (dict "addedKey" "value1") }}

If one later execute the code below, I think you are correct in that the right dictionary (.) would override the left and make addedKey have "value1" instead of "value2".

{{- $_ := merge (dict "addedKey" "value2") . }}

But doing...

{{- $addedKey := "something" }}

Will not be accessible through .addedKey but instead $addedKey.

It simply reformats "jupyterhub.matchLabels".
Used to by the pod-culler to select singleuser-server pods. It simply
reformats "jupyterhub.matchLabels" and sets the componentLabel value so
`component=singleuser-server` is outputted.
Copy link
Member

Choose a reason for hiding this comment

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

Used to by the -> Used by the
is outputted -> is output

@betatim
Copy link
Member

betatim commented Jul 6, 2018

Looks good to me. There are a few typos and some questions for my education.

When I read the comments before about scope I had assumed it was talking about things set in the template where the helper was being called. Somehow that confused me :)

@consideRatio consideRatio force-pushed the fix-pod-culler-selector-pr branch from 43bfb44 to f71fc5b Compare July 6, 2018 23:16
@consideRatio
Copy link
Member Author

@betatim thank you for helping me process this! I really appreciate it!

@betatim betatim merged commit 739e0a7 into jupyterhub:master Jul 8, 2018
@betatim
Copy link
Member

betatim commented Jul 8, 2018

🎉 !

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.

Pod culler is using the wrong label to watch pods
2 participants