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

feat(argo-cd): Support ability to set .Values.namespaceOverride #2679

Merged
merged 14 commits into from May 14, 2024

Conversation

andres-vara
Copy link
Contributor

@andres-vara andres-vara commented May 7, 2024

Resolves #2664

Checklist:

  • I have bumped the chart version according to versioning
  • I have updated the documentation according to documentation
  • I have updated the chart changelog with all the changes that come with this pull request according to changelog.
  • Any new values are backwards compatible and/or have sensible default.
  • I have signed off all my commits as required by DCO.
  • My build is green (troubleshooting builds).

…Binding (argoproj#2676)

remove unnecessary if statements

Signed-off-by: Daniel Beilin <daniel.beilin@outlook.com>
Co-authored-by: Aikawa <yu.croco@gmail.com>
Signed-off-by: Andres Vara Parsegov <andres.vara@chase.com>
Andres Vara Parsegov added 2 commits May 7, 2024 16:37
Signed-off-by: Andres Vara Parsegov <andres.vara@chase.com>
Signed-off-by: Andres Vara Parsegov <andres.vara@chase.com>
argoproj-renovate bot and others added 8 commits May 7, 2024 15:49
…argoproj#2677)

Co-authored-by: renovate[bot] <renovate[bot]@users.noreply.github.com>
Signed-off-by: Andres Vara Parsegov <andres.vara@chase.com>
argoproj#2678)

Signed-off-by: Andres Vara Parsegov <andres.vara@chase.com>
Signed-off-by: Andres Vara Parsegov <andres.vara@chase.com>
Signed-off-by: Andres Vara Parsegov <andres.vara@chase.com>
Signed-off-by: Andres Vara Parsegov <andres.vara@chase.com>
Signed-off-by: Andres Vara Parsegov <andres.vara@chase.com>
Signed-off-by: Andres Vara Parsegov <andres.vara@chase.com>
@mkilchhofer
Copy link
Member

Hi @andres-vara

Since your PR description misses context or a related issue link:
What is your motivation resp. your use case for this change?

@andres-vara
Copy link
Contributor Author

Hi @andres-vara

Since your PR description misses context or a related issue link: What is your motivation resp. your use case for this change?

Hi @mkilchhofer
Attached issue to the context.

In my case, I don't have access to .Release.Namespace via the CD tool we use, which is why I believe it's beneficial to have an alternative method of specifying the namespace using the values file.

@pdrastil
Copy link
Member

@andres-vara Hi thanks for the contribution but Helm natively supports helm install --namespace <target> flag so IMHO I don't see a need to have override via values.yaml. Why is this needed?

@tico24
Copy link
Member

tico24 commented May 13, 2024

@pdrastil I can answer this one.

If you use a chart as a dependent chart in another, you can't use the --namespace flag to put the dependent chart in a namespace different from the 'main' chart. This is extremely frustrating.

@pdrastil
Copy link
Member

@tico24 Question is why would you want to do that. You don't deploy server / repo-server / each controller into separate namespace. In my experienece if you need more than 1 namespace then you should have 2 independent deployments instead of 1 huge umbrella chart as each namespace provides isolation (and if you have Cilium / NetPolicies the namespace override will not work reasonably well anyway).

@tico24
Copy link
Member

tico24 commented May 13, 2024

I wouldn't want to do that. But I would want to deploy (in my case) argo workflows and another application in the same helm chart, but in different namespaces.

I can easily see a requirement to deploy argocd and another application in one helm chart but in different namespaces.

We did discuss this quite a bit in the team slack when the topic first came up. There's strong prescidence out there for doing it, even though it feels hacky (mostly because helm sucks with support for its own dependent chart deployment options).

@andres-vara
Copy link
Contributor Author

@andres-vara Hi thanks for the contribution but Helm natively supports helm install --namespace <target> flag so IMHO I don't see a need to have override via values.yaml. Why is this needed?

As mentioned earlier, if your company policy requires all helm releases to be in one default namespace, '.Release.Namespace' may not be available for modification. There is no harm in having an additional option to manage the deployment namespace via values

@mkilchhofer
Copy link
Member

mkilchhofer commented May 13, 2024

I think we can accept it, @tico24 found the context we already accepted some months ago:

@pdrastil
Copy link
Member

Ok thanks for sharing context and I'm fine with having extra way of managing things.

Signed-off-by: Andres Vara <46708607+andres-vara@users.noreply.github.com>
Signed-off-by: Andres Vara Parsegov <andres.vara@chase.com>
@yu-croco
Copy link
Collaborator

Hi @andres-vara

I have updated the chart changelog with all the changes that come with this pull request according to changelog.

can you please update (remove old ones and add new one) changelog in Chart.yaml ?
Ref: https://github.com/argoproj/argo-helm/pull/2679/files#diff-16f38cd1a4674cb682ac9f015fbc1c1ff552f024a8f791c16de0de21a1f65771R28-R34

Signed-off-by: Andres Vara <46708607+andres-vara@users.noreply.github.com>
@mbevc1 mbevc1 merged commit 7be9b01 into argoproj:main May 14, 2024
7 checks passed
vitorscassiano pushed a commit to vitorscassiano/argo-helm that referenced this pull request May 16, 2024
…proj#2679)

* feat(argo-workflows): Allow adding additional ServiceAccounts to RoleBinding (argoproj#2676)

remove unnecessary if statements

Signed-off-by: Daniel Beilin <daniel.beilin@outlook.com>
Co-authored-by: Aikawa <yu.croco@gmail.com>
Signed-off-by: Andres Vara Parsegov <andres.vara@chase.com>

* feat(argo-cd): Support ability to set .Values.namespaceOverride

Signed-off-by: Andres Vara Parsegov <andres.vara@chase.com>

* fix(argo-cd): typo

Signed-off-by: Andres Vara Parsegov <andres.vara@chase.com>

* chore(deps): update actions/create-github-app-token action to v1.10.0 (argoproj#2677)

Co-authored-by: renovate[bot] <renovate[bot]@users.noreply.github.com>
Signed-off-by: Andres Vara Parsegov <andres.vara@chase.com>

* feat(argo-rollouts): Add podLabels at the controller & dashboard level (argoproj#2678)

Signed-off-by: Andres Vara Parsegov <andres.vara@chase.com>

* feat(argo-cd): Support ability to set .Values.namespaceOverride

Signed-off-by: Andres Vara Parsegov <andres.vara@chase.com>

* fix(argo-cd): typo

Signed-off-by: Andres Vara Parsegov <andres.vara@chase.com>

* fix(argo-cd): autocorrection

Signed-off-by: Andres Vara Parsegov <andres.vara@chase.com>

* fix(argo-cd): typos

Signed-off-by: Andres Vara Parsegov <andres.vara@chase.com>

* fix(argo-cd): typos

Signed-off-by: Andres Vara Parsegov <andres.vara@chase.com>

* removed auota

Signed-off-by: Andres Vara Parsegov <andres.vara@chase.com>

* Update Chart.yaml

Signed-off-by: Andres Vara <46708607+andres-vara@users.noreply.github.com>

---------

Signed-off-by: Daniel Beilin <daniel.beilin@outlook.com>
Signed-off-by: Andres Vara Parsegov <andres.vara@chase.com>
Signed-off-by: Andres Vara <46708607+andres-vara@users.noreply.github.com>
Co-authored-by: Daniel Beilin <144586547+dbeilin@users.noreply.github.com>
Co-authored-by: Aikawa <yu.croco@gmail.com>
Co-authored-by: Andres Vara Parsegov <andres.vara@chase.com>
Co-authored-by: argoproj-renovate[bot] <161757507+argoproj-renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <renovate[bot]@users.noreply.github.com>
Co-authored-by: mitchell amihod <mitchell@amihod.com>
Signed-off-by: vitor.cassiano <vitor.cassiano@picpay.com>
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.

Make Helm chart take namespace via values.yaml
8 participants