-
Notifications
You must be signed in to change notification settings - Fork 113
Add script functions for reporting Kubernetes manifests from scripts #1474
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
base: main
Are you sure you want to change the base?
Conversation
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.
Changes look good to me, just a few nits for your consideration.
Love to see the testing - it's got me wondering if we should add tests for some of the poorly crafted yaml scenarios you identified in the PR description (eg. extra spaces before document separators, excess newlines, double document separators without content between, etc.).
I'd be content knowing you've tested these manually if you think adding them as test cases is overkill.
manifest=""" | ||
\"apiVersion\": \"v1\" | ||
\"kind\": \"Namespace\" | ||
\"metadata\": | ||
\"name\": \"example\" | ||
\"labels\": | ||
\"name\": \"example\" | ||
--- | ||
\"apiVersion\": \"v1\" | ||
\"kind\": \"Namespace\" | ||
\"metadata\": | ||
\"name\": \"diffs\" | ||
\"labels\": | ||
\"name\": \"diffs\" | ||
""" |
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.
Using Heredoc can clean up those escapes (disclaimer, I didn't test this)
read -r -d '' manifest << EOM
"apiVersion": "v1"
"kind": "Namespace"
"metadata":
"name": "example"
"labels":
"name": "example"
---
"apiVersion": "v1"
"kind": "Namespace"
"metadata":
"name": "diffs"
"labels":
"name": "diffs"
EOM
|
||
MANIFESTS+=("$CURRENT") | ||
|
||
for i in "${MANIFESTS[@]}"; do |
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.
naming nit since it's not an index
for i in "${MANIFESTS[@]}"; do | |
for manifest in "${MANIFESTS[@]}"; do |
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.
nit for next time: consider applying the formatting in a separate commit before the change
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.
yeah, I didn't mean to re-format but it did it :shakes-fist:
Adds new functions to both the PowerShell and Bash bootstrap scripts to allow for reporting of kubernetes manifests.
There are two functions
Report-KubernetesManifest
/report_kubernetes_manifest
which takes two parameters, the manifest content and optionally, the namespace.This function splits the manifest on
---
(with/without trailing spaces) and registers each document separately using the service message.If a manifest contains multiple documents and a namespace is provided, all reported manifests get the same namespace
Report-KubernetesManifestFile
/report_kubernetes_manifest_file
which takes two parameters, the full path to the manifest file and optionally, the namespace.This function reads the contents of the file and then calls the function above.