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

helm/web-deployment: add env vars for log level of autotrace_webhook #75

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chibby0ne
Copy link

These environment variables come useful during debugging when the autotrace
webhook is used.

Signed-off-by: Antonio Gutierrez [email protected]

These environment variables come useful during debugging when the autotrace
webhook is used.

Signed-off-by: Antonio Gutierrez <[email protected]>
Copy link
Contributor

@nfisher nfisher left a comment

Choose a reason for hiding this comment

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

Suggested changes but otherwise LGTM.

# autotrace-webhook binaries to log in debug level
debug_autotrace: false
# For the mini ones minikube, minishift set to true
nodeport: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
nodeport: false
nodeport: 0

Rather than a boolean I would make it a "optional" int value so the port can be adjusted to whatever the user wants. With a 0 value the nodePort would be disabled. Also Helm discourages heavy nesting. I'd probably opt for webDebugAutotrace and webNodePort. If we wanted dynamic assignment by the cluster of a nodePort then it might make sense to have a boolean and a numeric but it's probably over-engineering for a demo app.

@@ -11,7 +11,7 @@ spec:
targetPort: 8080
selector:
service: web
{{ if .Values.nodeport }}
{{ if .Values.web.nodeport }}
Copy link
Contributor

Choose a reason for hiding this comment

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

No change required if this is numeric.

@@ -11,7 +11,7 @@ spec:
targetPort: 8080
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
targetPort: 8080
{{ if .Values.web.nodeport }}
nodePort: {{ .Values.web.nodeport }}
{{ else }}
targetPort: 8080
{{ end }}


web:
# autotrace-webhook binaries to log in debug level
debug_autotrace: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
debug_autotrace: false
debugAutotrace: false

Ideally we'd maintain consistency with the rest of the config and not use snake case.

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