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

Fixes dex for kustomize 5 #2586

Merged
merged 3 commits into from
Jan 2, 2024
Merged

Fixes dex for kustomize 5 #2586

merged 3 commits into from
Jan 2, 2024

Conversation

alekseyolg
Copy link
Contributor

While the command is running:

kustomize build common/dex/overlays/istio

A warning appeared:

# Warning: 'vars' is deprecated. Please use 'replacements' instead. [EXPERIMENTAL] Run 'kustomize edit fix' to update your Kustomization automatically.

I used the command:

kustomize edit fix --vars

And also corrected errors received when executing this command. Manifests are now generated without warnings.

Please note that this PR should not affect the manifests themselves in any way, only the warning is removed.

Resolves #

Description of your changes:

Checklist:

  • Unit tests pass:
    Make sure you have installed kustomize == 3.2.1
    1. make generate-changed-only
    2. make test

@juliusvonkohout
Copy link
Member

@kromanow94 you could test/diff this as well.

@kromanow94
Copy link
Contributor

I was able to confirm that the output manifests from this PR and from latest master are exactly the same:

$ git remote add alekseyolg https://github.com/alekseyolg/manifests.git
$ git fetch alekseyolg
$ git checkout alekseyolg/master -b 2586-alekseyolg-fixes-dex-for-kustomize-5
$ git log -1
commit 236d0fb10fdc823b9da4efb927bf9e25fb1bae2a (HEAD -> 2586-alekseyolg-fixes-dex-for-kustomize-5, alekseyolg/master)
Author:     Aleksey Karpov <[email protected]>
AuthorDate: Mon Jan 1 17:23:56 2024 +0300
Commit:     GitHub <[email protected]>
CommitDate: Mon Jan 1 17:23:56 2024 +0300

    Fixes dex for kustomize 5

$ kustomize  build common/dex/overlays/istio/ > 2586.dex.overlay.istio.yaml
$ git checkout master
$ git log -1
commit cd991e6010a8c694a729d2978f4c8be597669fff (HEAD -> master, origin/master, origin/HEAD)
Author:     Max Voitko <[email protected]>
AuthorDate: Tue Dec 26 21:08:09 2023 +0100
Commit:     GitHub <[email protected]>
CommitDate: Tue Dec 26 20:08:09 2023 +0000

    Use hashFromEnv in dex component for static user passwords (#2229)

$ kustomize  build common/dex/overlays/istio/ > master.dex.overlay.istio.yaml
$ diff master.dex.overlay.istio.yaml 2586.dex.overlay.istio.yaml
# NOTE(kromanow94): This presents empty output so no changes.

That said, from functional perspective I don't have anything to add. But, I suggest some small improvements related to readability.
@alekseyolg can you make a few changes so each section is separated by a newline and the line namespace: auth is directly after the line kind: Kustomization?

That's how I'd do it:

diff --git a/common/dex/overlays/istio/kustomization.yaml b/common/dex/overlays/istio/kustomization.yaml
index 15ed5af7..4c522c1c 100644
--- a/common/dex/overlays/istio/kustomization.yaml
+++ b/common/dex/overlays/istio/kustomization.yaml
@@ -1,11 +1,14 @@
 apiVersion: kustomize.config.k8s.io/v1beta1
 kind: Kustomization
+namespace: auth
+
 resources:
 - ../../base
 - virtual-service.yaml
-namespace: auth
+
 configurations:
 - params.yaml
+
 replacements:
 - source:
     fieldPath: metadata.name

@juliusvonkohout
Copy link
Member

/lgtm
/approve

@kromanow94 just create a follow up PR, that will be faster.

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alekseyolg, juliusvonkohout

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit ed4caf3 into kubeflow:master Jan 2, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants