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

Replace template parsing with yaml.Decoder #42

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

Conversation

philk
Copy link

@philk philk commented Aug 28, 2024

This solves some of the edge cases around parsing for empty or invalid document by replacing the string splitting with the yaml package's native handling of multi-document files. Along the way it also simplifies upstream functions by returning parsed yaml as a map[string]interface{} instead of a string. Even after #29 I still had issues with documents like:

---
# <snip, normal document>
          env:
            - name: "GOMAXPROCS"
              value: "5"
            - name: "GOMEMLIMIT"
              value: "6442450944"
            - name: "JAEGER_REPORTER_MAX_QUEUE_SIZE"
              value: "1000"
---
# Source: mimir/charts/mimir-distributed/templates/minio/create-bucket-job.yaml
# Minio provides post-install hook to create bucket
# however the hook won't be executed if helm install is run
# with --wait flag. Hence this job is a workaround for that.
# See https://github.com/grafana/mimir/issues/2464
---
# Source: mimir/charts/mimir-distributed/templates/metamonitoring/mixin-alerts.yaml
apiVersion: monitoring.coreos.com/v1
kind: PrometheusRule
metadata:
  name: mimir-alerts
# <etc>

In the event of an un-parseable document you'll receive an error with the parsing error and the document the parsing failed on. This isn't extremely useful and returning the chunk around the failure seems like it'd be more useful, but that's beyond my skills and this should point folks in the right direction at least.

One other note is I had to fix a test error that currently exists in master (see 7d26eaf) with the image-regex functionality. Along the way I spent a little time digging in and I'm pretty sure that it doesn't work at all since none of the Get calls (see Deployments) are passing through the regex except ConfigMap. I have a partial branch that attempts to make image-regex work but didn't want to block this fix with that.

Phil Kates added 2 commits August 27, 2024 20:51
This solves some of the edge cases around parsing for empty or invalid
document by replacing the string splitting with the yaml package's
native handling of multi-document files.

Along the way it also simplifies upstream functions by returning parsed
yaml as a map[string]interface{} instead of a string.
These tests should probably be replaced with a more thorough testing of
the image-regex feature, which appears to currently be broken. This
doesn't break anything and is at least a step in the right direction.
@nikhilsbhat
Copy link
Owner

@philk , thanks for the PR. I'll review this and update.

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