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

fix(#137): use nullable boolean type for the Release boolean fields #707

Merged
merged 1 commit into from
Sep 1, 2022

Conversation

prizov
Copy link
Contributor

@prizov prizov commented Aug 31, 2022

Why

The changes fix an issue when Helmsman doesn't overwrite positive boolean values with negatives for Helm releases while merging multiple DSFs(#137).

What

  • Adding a new type NullBool (inspired by NullBool tyoe from sql package)
  • Adding a mergo transormer with custom merging logic(overwriting) for the new type
  • Changing the Release bool fields to the NullBool type
  • Adding corresponding tests
  • Adjusting the JSON schema. The NullBool type is considered as boolean type, so there are no changes from the usage perspective

Verification

make test + verified manually

Example

charts/common.yaml

helmRepos:
  jenkins: https://charts.jenkins.io

namespaces:
  staging:

apps:
  jenkins:
    namespace: staging
    enabled: true
    chart: jenkins/jenkins
    version: 2.15.1

charts/custom.yaml

apps:
  jenkins:
    enabled: false

charts/spec.yaml

stateFiles:
  - path: ./common.yaml
  - path: ./custom.yaml

Expected behavior

The jenkins app has enabled field set to false and accordingly Helmsman shouldn't install this release

Current behavior

➜ helmsman --debug --spec spec.yaml
...
Applications: 
--------------- 

        name:  jenkins
        description:  
        namespace:  staging
        enabled:  true
        chart:  jenkins/jenkins
        version:  2.15.1
...
2022-08-31 19:48:06 NOTICE: -------- PLAN starts here --------------
2022-08-31 19:48:06 NOTICE: Release [ jenkins ] in namespace [ staging ] will be installed using version [ 2.15.1 ] -- priority: 0
2022-08-31 19:48:06 NOTICE: -------- PLAN ends here --------------

With proposed changes

➜ helmsman --debug --spec spec.yaml
...
Applications: 
--------------- 

        name:  jenkins
        description:  
        namespace:  staging
        enabled:  false
        chart:  jenkins/jenkins
...
2022-08-31 19:49:03 NOTICE: -------- PLAN starts here --------------
2022-08-31 19:49:03 INFO: Release [ jenkins ] in namespace [ staging ]is disabled -- priority: 0
2022-08-31 19:49:03 NOTICE: -------- PLAN ends here --------------

Copy link
Collaborator

@luisdavim luisdavim left a comment

Choose a reason for hiding this comment

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

🥇 Thanks

@luisdavim luisdavim merged commit 982bd52 into Praqma:master Sep 1, 2022
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