microsoft / fabrikate

Making GitOps with Kubernetes easier one component at a time
MIT License
38 stars 5 forks source link

Invalid YAML removes most of the content of HLD #241

Closed Lybecker closed 5 years ago

Lybecker commented 5 years ago

Describe the bug: Adding an environment variable with no space between the key and the value removes most of the content of the HLD YAML file.

To Reproduce:

  1. Make a /config/common.yaml like this:

    subcomponents:
    anotherservice:
    config:
      image:
        tag: "744"
      replicaCount: 1
    screenprofiler:
    config:
      envVar:
        important: true
        fabshouldfail: true
        is_this_a_bug: definitely
        culprit:"this is where it all goes wrong due to the lack of space between key and value"
      image:
        repository: somewheregood.azurecr.io/screenprofiler
        tag: "73"
      replicaCount: 1

    The subcomponents | screenprofiler | envVar | culprit is the problem

  2. Execute fabricate

    ./fab set --subcomponent screenprofiler image.tag=747
  3. Fabricate removes most of the HLD file:

    subcomponents:
    screenprofiler:
    config:
      image:
        tag: "747"

Expected behavior: Fabricate should fail, as the YAML is invalid

OR

subcomponents | screenprofiler | image | tag is set to 747

Like

subcomponents:
  anotherservice:
    config:
      image:
        tag: "744"
      replicaCount: 1
  screenprofiler:
    config:
      envVar:
        important: true
        fabshouldfail: true
        is_this_a_bug: definitely
        culprit:"this is where it all goes wrong due to the lack of space between key and value"
      image:
        repository: somewheregood.azurecr.io/screenprofiler
        tag: "747"
      replicaCount: 1

Desktop (please complete the following information):

Reproduced on:

fab version 0.15.2

Execute via Azure DevOps Agent like so: curl https://raw.githubusercontent.com/Microsoft/bedrock/master/gitops/azure-devops/build.sh > build.sh chmod +x ./build.sh

curl https://raw.githubusercontent.com/Microsoft/bedrock/master/gitops/azure-devops/release.sh > release.sh chmod +x ./release.sh

timfpark commented 5 years ago

I will investigate - I was able to repro with the config file provided.

timfpark commented 5 years ago

Root cause: Fabrikate is unable to distinguish right now between a failed file load (which causes it to move on to JSON) and a failed YAML file parse.

Fix: Add support for differentiating between these two failures and fatally failing if the YAML or JSON file parse fails in componentConfig.go and add tests to cover.

timfpark commented 5 years ago

PR #242 contains fix and tests to cover.

timfpark commented 5 years ago

The same issue happens for malformed component files - adding checks for this as well.

Lybecker commented 5 years ago

Thanks!

I have verified the fix.

Nice solution with informative errors like: