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: Quick hack to address issue 68 - snapshot tag doesn't meet strict semver check #390

Conversation

dee0sap
Copy link
Contributor

@dee0sap dee0sap commented Mar 26, 2024

Description

In issue #68 I described trying to make this flow
ComponentVersion -> Localization -> Configuration -> FluxDeployer
, where the helm chart is a resource with in the componentversion, work without success.

One of the problems is that the tag generated by the Configuration snapshot is not a valid semvar. So even if you use the extraIdentity helmChart= to make it so the snapshot repo will be named in a way helm will accept, see here, helm still will complain cause tag is just the k8s revision of the object owning the snapshot.

At the moment I am just tacking on ".0.0" to keep helm from complaining when the extraidentity helmChart= is present. Ideally I would like to have the tag come from version information specified for the resource within the compontversion itself. This because some day helm may complain if it finds that the Chart.yaml is specifying a different version than the repo tag is suggesting.

Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.

What type of PR is this? (check all applicable)

  • 🍕 Feature
  • [x ] 🐛 Bug Fix
  • 📝 Documentation Update
  • 🎨 Style
  • 🧑‍💻 Code Refactor
  • 🔥 Performance Improvements
  • ✅ Test
  • 🤖 Build
  • 🔁 CI
  • 📦 Chore (Release)
  • ⏩ Revert

Related Tickets & Documents

  • Related Issue # 68
  • Closes # (issue)
  • Fixes # (issue)

Remove if not applicable

Screenshots

Added tests?

  • 👍 yes
  • 🙅 no, because they aren't needed
  • 🙋 no, because I need help
  • Separate ticket for tests # (issue/pr)

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Added to documentation?

  • 📜 README.md
  • 🙅 no documentation needed

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@dee0sap dee0sap marked this pull request as draft March 26, 2024 06:13
@Skarlso
Copy link
Contributor

Skarlso commented Mar 26, 2024

Can you update the link please to the right issue? It was probably moves that's why it's pointing at a pull request right now.

I'll look at this next, but since helm was already working fine, I'm not sure this is needed. But I'll run a scenario today.

@Skarlso
Copy link
Contributor

Skarlso commented Mar 26, 2024

I think you mean this: open-component-model/ocm-project#68

@Skarlso
Copy link
Contributor

Skarlso commented Mar 26, 2024

Screenshot 2024-03-26 at 13 02 47

Okay, so this is working as you expect:
https://github.com/Skarlso/ocm-helm/tree/main/components

Here are the files:

components_version.yaml

apiVersion: delivery.ocm.software/v1alpha1
kind: ComponentVersion
metadata:
  name: ocm-with-helm
  namespace: ocm-system
spec:
  interval: 10m0s
  component: github.com/skarlso/podinfo-helm
  version:
    semver: 1.0.0
  repository:
    url: ghcr.io/skarlso
    secretRef:
      name: creds

Resource:

apiVersion: delivery.ocm.software/v1alpha1
kind: Resource
metadata:
  name: ocm-with-helm-deployment
  namespace: ocm-system
spec:
  interval: 10m
  sourceRef:
    kind: ComponentVersion
    name: ocm-with-helm
    namespace: ocm-system
    resourceRef:
      name: charts
      version: 6.3.5
      extraIdentity:
        helmChart: podinfo

And then the deployer:

apiVersion: delivery.ocm.software/v1alpha1
kind: FluxDeployer
metadata:
  name: fluxdeployer-podinfo-pipeline-backend
  namespace: ocm-system
spec:
  interval: 1m0s
  sourceRef:
    kind: Resource
    name: ocm-with-helm-deployment
  helmReleaseTemplate:
    chart:
      spec:
        chart: podinfo
        version: 6.3.5
    interval: 5m

The generated snapshot's URL will be something like this:

Status:
  Conditions:
    Last Transition Time:  2024-03-26T12:01:45Z
    Message:               Snapshot with name 'ocm-with-helm-deployment-mw73dth' is ready
    Observed Generation:   1
    Reason:                Succeeded
    Status:                True
    Type:                  Ready
  Digest:                  sha256:31b957024d57c2b4768c2a5ff9004fe0acdbbcf6974167fa0a95d858c21ea985
  Observed Generation:     1
  Repository URL:          https://registry.ocm-system.svc.cluster.local:5000/sha-785370814667106374/podinfo
  Tag:                     6.3.5

And you see that the tag is the right version.

I'll try a configuration/localization one next.

@Skarlso
Copy link
Contributor

Skarlso commented Mar 26, 2024

Okay, so yes. The configuration and localization functionality for helm charts is missing because it doesn't re-package it properly and creates a new helm chart out of it.

This is a missing feature, basically. We didn't quite have the time to finish it.

@morri-son morri-son added this to the 2024-Q2 milestone Apr 19, 2024
Skarlso
Skarlso previously approved these changes Apr 20, 2024
@Skarlso Skarlso marked this pull request as ready for review April 20, 2024 05:33
@Skarlso Skarlso enabled auto-merge (squash) April 20, 2024 06:19
@Skarlso Skarlso disabled auto-merge April 20, 2024 06:24
@Skarlso Skarlso merged commit 7c50983 into open-component-model:main Apr 20, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔒Closed
Development

Successfully merging this pull request may close these issues.

3 participants