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): Add option to disable API checks in all servicemonitor #2729

Closed
wants to merge 13 commits into from

Conversation

psychomantys
Copy link

@psychomantys psychomantys commented May 31, 2024

What this PR does / why we need it:

This helm attempts to verify the API version during deployment processes. However, in some environments, direct access to the API is not available beforehand, which complicates the deployment.

This situation is particularly problematic in two notable scenarios:

  • Offline Manifest Generation and Application: When the manifests are generated in one environment (e.g., a CI/CD pipeline or a local machine) and then applied to another cluster, there's no way to verify the API version during the generation phase.
  • Manifest Generation Using Kustomize: In environments where manifests are created using tools like kustomize, the version check becomes redundant and can lead to deployment issues.

My PR proposes modifications that bypass the API version check under these specific conditions, ensuring smoother and more flexible deployments in environments lacking direct API connectivity.

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).

@psychomantys psychomantys changed the title Patch 1 Add option to servicemonitor disable api checks May 31, 2024
@psychomantys psychomantys changed the title Add option to servicemonitor disable api checks feat: Add option to servicemonitor disable api checks May 31, 2024
@psychomantys psychomantys changed the title feat: Add option to servicemonitor disable api checks feat: Add option to disable API checks in all servicemonitor May 31, 2024
Signed-off-by: Psycho Mantys <psycho.mantys@gmail.com>
Signed-off-by: Psycho Mantys <psycho.mantys@gmail.com>
Signed-off-by: Psycho Mantys <psycho.mantys@gmail.com>
Signed-off-by: Psycho Mantys <psycho.mantys@gmail.com>
Signed-off-by: Psycho Mantys <psycho.mantys@gmail.com>
Signed-off-by: Psycho Mantys <psycho.mantys@gmail.com>
Signed-off-by: Psycho Mantys <psycho.mantys@gmail.com>
Signed-off-by: Psycho Mantys <psycho.mantys@gmail.com>
Signed-off-by: Psycho Mantys <psycho.mantys@gmail.com>
Signed-off-by: Psycho Mantys <psycho.mantys@gmail.com>
Signed-off-by: Psycho Mantys <psycho.mantys@gmail.com>
Signed-off-by: Psycho Mantys <psycho.mantys@gmail.com>
Signed-off-by: Psycho Mantys <psycho.mantys@gmail.com>
@psychomantys psychomantys changed the title feat: Add option to disable API checks in all servicemonitor feat(argo-cd): Add option to disable API checks in all servicemonitor May 31, 2024
@mkilchhofer
Copy link
Member

mkilchhofer commented May 31, 2024

Hi @psychomantys
Thanks for your contribution. I apreciate everybody who helps us to make the chart better and better.

I am not a fan of this approach. We had this request already one time and declined it.
Background is that all tools around helm (Kustomize, Helmfile, Argo CD, ...) have the option to pass in all the available APIs of kube (CRDs, Core API versions, etc).

Offline Manifest Generation and Application

For this, you need to execute helm with the parameter --api-versions:

helm template argocd \
  oci://ghcr.io/argoproj/argo-helm/argo-cd \
  --api-versions monitoring.coreos.com/v1 \
  --values argocd-values.yaml

Manifest Generation Using Kustomize

There is first class support for this also in kustomize:

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization

helmCharts:
- name: argo-cd
  repo: oci://ghcr.io/argoproj/argo-helm
  version: 7.0.0
  releaseName: argocd
  apiVersions:                 #
  - monitoring.coreos.com/v1   # see this structure
  valuesInline:
    controller:
      metrics:
        enabled: true
        serviceMonitor:
          enabled: true
    server:
      metrics:
        enabled: true
        serviceMonitor:
          enabled: true

    repoServer:
      metrics:
        enabled: true
        serviceMonitor:
          enabled: true

Update:
Oh and here is even an example on howto do it in helmfile:

repositories:
  - name: argo
    url: https://argoproj.github.io/argo-helm

apiVersions:
  - monitoring.coreos.com/v1

releases:
  - name: argocd
    namespace: argocd
    chart: argo/argo-cd
    values:
      - argocd-values.yaml

@psychomantys
Copy link
Author

Thank you for your contribution and for sharing your perspective!

I understand your point of view, and i agree that relying solely on external tools for api checks is ideal for wider range of use cases. To address this, I've implemented the functionality with an optional variable. By default, the functionality will be disabled, preserving the current behavior and ensuring compatibility with existing workflows.

For users who prefer this approach, the variable can be easily enabled in the values.yaml file. This means no other changes are required for Applications using methods like Kustomize, Helmfile or helm.

This approach aims to strike a balance between providing flexibility and maintaining compatibility with existing tooling and workflows.

@mkilchhofer
Copy link
Member

I understand your point of view, and i agree that relying solely on external tools for api checks is ideal for wider range of use cases. To address this, I've implemented the functionality with an optional variable.

I think I was clear with my explanation, no? 😃

We already discussed this inside the maintainers chat and we'd like to decline the idea.
Our opinion is that this is a redundant implentation of a feature which is already provided by Helms core functionality.

I mean it is hard to say "no", we really appreciate every contribution. But supporting all edge cases of users also means more work (due to more testing) for us.
I hope you understand our decision?

Back to topic? What tool(s) are you using?
Maybe we can support you, finding the right parameter for your tool?

Copy link

github-actions bot commented Aug 1, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

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.

2 participants