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 Kustomize deprecations #335

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

benwh
Copy link
Contributor

@benwh benwh commented Aug 19, 2024

Here's a couple of fixes that we've got in our fork. Details in commit messages.

@0x0013
Copy link
Contributor

0x0013 commented Nov 26, 2024

Hi @benwh, thanks for this PR.

It seems though that integration test fails after this change. From what I can see, since we are no longer defining the environment variable THEATRE_IMAGE, but rather replace it directly, the vault manager overlay used for acceptance test, which still overlays the command args with the env variable, now results in an invalid read-a-secret-* container, as the image: value no longer corresponds to an existing environment variable.

We still need to define the --theatre-image= in the overlay, otherwise it will inherit the (production) value from the base, so it can't just be removed from the overlay.

I don't think reading the THEATRE_IMAGE value is the purpose of the tests, so it should be fine to directly replace it in the acceptance overlay with the test image used:

# ...
          args:
            - --theatre-image=theatre:latest
            - --metrics-address=0.0.0.0
# ...

I see the tests are failing in your fork as well, though the logs have timed out, I assume the reason is the same.

@0x0013
Copy link
Contributor

0x0013 commented Nov 26, 2024

@benwh also note that we've merged #338 to update Go version, and with it, we've pinned envtest to 0.19, thus 5975be9 would be unneeded as it pins to an earlier version.

@benwh benwh force-pushed the ben-upstream-kustomize-deprecations branch from 62d067d to 8ed20a3 Compare November 27, 2024 18:43
When running `kustomize build`, we currently see some warnings. Get rid
of these by using the more modern replacements. See [docs][0].

```
Warning: 'commonLabels' is deprecated. Please use 'labels' instead.
Warning: 'vars' is deprecated. Please use 'replacements' instead.
```

[0]: https://kubectl.docs.kubernetes.io/references/kustomize/kustomization/
@benwh benwh force-pushed the ben-upstream-kustomize-deprecations branch from 8ed20a3 to dc1684f Compare November 27, 2024 18:44
@benwh
Copy link
Contributor Author

benwh commented Nov 27, 2024

@0x0013 thanks for the pointers!

I've rebased to drop the superseded commit, and updated the acceptance kustomization to fix what I suspect are the issues there (I've been somewhat ignoring the vault-manager stuff, because we don't use that here)

@benwh benwh changed the title Fix CI dependencies and Kustomize deprecations Fix Kustomize deprecations Nov 27, 2024
Copy link
Contributor

@0x0013 0x0013 left a comment

Choose a reason for hiding this comment

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

@benwh Interesting way to solve the acceptance tests issue, by repeating the whole replacement. Though it produces more overlay code, I agree that it's a better approach than just hardcoding the acceptance image in the overlay (- --theatre-image=theatre:latest) as now we use the same replacement pattern in tests as well.

I've left a small suggestion which would hopefully help avoid divergent code in the future.

@@ -22,16 +24,27 @@ resources:
- rbac/leader-election.yaml
- cert-manager/certificate.yaml

vars:
replacements:
Copy link
Contributor

Choose a reason for hiding this comment

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

with the new approach of repeating this replacement in overlay kustomization, there is a slight risk of someone modifying this replacement code, but forgetting to mirror the modification in overlay, in which case the replacement could be buggy, and the tests would still pass.

I don't think there's an elegant way to solve this, but could you add a code comment that the replacement code needs to be mirrored in config/acceptance/kustomization.yaml?

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