fix: clean balancer

[osamamagdy]

Thank you for submitting a pull request to the WrongSecrets Party!

What kind of changes does this PR include?

• Fixes or refactors
  It is a partial fix to Cleanup helm chart #86

Checklist:

• All the contributions made are solely the work of me and my co-authors
• I tested the changes in this PR (if applicable)
• I added tests to ensure my change works (if applicable)
• The PR passes pre-commit hooks and automated tests

Description:

• Added package.json so we can test with some frequent commands like helm template and helm install --dry-run which will be npm run template and npm run dry-run respectively.
• Added a test-values.yaml file to try how values can be overridden by other values different than the original values.yaml
• Generating test.tmp.yaml which is the generated k8s manifests by helm to detect any changes I can make by mistake in the future ( we don't want to change the generated manifests during the clean-up process)
• Removing hardcoded securityContext from the balancer deployment in both the container specs and pod specs (taken from values files)
• Making the env section a loop that iterates over all env values in values files

[osamamagdy]

The file test.tmp.yaml fails the pre-commit check as it contains multiple files. Can we make this file the only exception?

[commjoen]

Hi @osamamagdy ! Love the steps forward!
Here are my 2 cents. @bendehaan can you please have a look at them and see if you agree ? (Eg balancer things in deployment and helm chart, while namespaced things need to undergo exactly this transformation to all things config.json). And then maybe extract more from code (hardcoded) for the namespaced objects and move to the config.json?)

[bendehaan]

The file test.tmp.yaml fails the pre-commit check as it contains multiple files. Can we make this file the only exception?

We could specifically set --allow-multiple-documents just for this file. That would mean changing the pre-commit config to something like this (note, untested 🙃):

      - id: check-yaml
        exclude: ^helm/wrongsecrets-ctf-party/templates/|^test.tmp.yaml)
      - id: check-yaml
        include: ^test.tmp.yaml
        with: 
          - --allow-multiple-documents

[osamamagdy]

The file test.tmp.yaml fails the pre-commit check as it contains multiple files. Can we make this file the only exception?

We could specifically set --allow-multiple-documents just for this file. That would mean changing the pre-commit config to something like this (note, untested 🙃):

      - id: check-yaml
        exclude: ^helm/wrongsecrets-ctf-party/templates/|^test.tmp.yaml)
      - id: check-yaml
        include: ^test.tmp.yaml
        with: 
          - --allow-multiple-documents

@bendehaan I applied those changes but I gues it won't take effect until you merge it first

[bendehaan]

I applied those changes but I gues it won't take effect until you merge it first

@osamamagdy made some changes to the pre-commit config, should work now :)

[commjoen]

Can you please update with master, resolve teh conflicts so we can do the final review please?

[osamamagdy]

Can you please update with master, resolve teh conflicts so we can do the final review please?

@commjoen Done

[bendehaan]

Nice progress! Could you add the two test files to gitignore? You don't have to delete them, but that way they won't clutter the public repository.

[osamamagdy]

Nice progress! Could you add the two test files to gitignore? You don't have to delete them, but that way they won't clutter the public repository.

@bendehaan If there are no other issues with this PR, I will delete them completely before merging (not needed with closing the PR).
@commjoen Are there any other issues?

[osamamagdy]

@bendehaan I deleted the two tests files