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

Add version-check command for vendored helm charts #1018

Merged
merged 4 commits into from
May 2, 2024

Conversation

SimKev2
Copy link
Contributor

@SimKev2 SimKev2 commented Apr 19, 2024

What are we doing

This change is a part of a larger effort to enable more automatic helm chart version management. First step is to get a command that can search the repositories within a chartfile.yaml for newer versions of the required charts. This output should be easy to parse for another script in order to kick off an automation to add the chart, vendor it, and make a PR.

Testing

Adding 2 different flux2 requires returns only 1 chart update :

$ ../../../../../go/src/github.com/grafana/tanka/tk tool charts version-check | jq
[
  {
    "name": "fluxcd-community/flux2",
    "version": "2.12.4",
    "app_version": "2.2.3",
    "description": "A Helm chart for flux2"
  }
]

Potential question here, should we be searching the existing requires for this version and exclude it if so? Right now if you have versions 2.12.4 and 2.12.3 of flux2 it would still return this even though you have the most up to date version.

Private repositories, I modified the chartfile.yaml to be a lower version to test it can search private repos:

$ ../../../../../go/src/github.com/grafana/tanka/tk tool charts version-check --repository-config $HOME/Library/Preferences/helm/repositories.yaml | jq
[
  {
    "name": "simkev2/nginx-ingress",
    "version": "1.2.0",
    "app_version": "3.5.0",
    "description": "NGINX Ingress Controller"
  }
]

@SimKev2 SimKev2 requested a review from julienduchesne April 19, 2024 20:10
Copy link
Contributor

github-actions bot commented Apr 19, 2024

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-05-02 18:52 UTC

Comment on lines 325 to 336
for _, sv := range searchVersions {
exists := false
for _, cv := range chartVersions {
if sv == cv {
exists = true
break
}
}
if !exists {
chartVersions = append(chartVersions, sv)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if keeping the same order as the requires would be easier to work with

The main goal is to enable our internal workflow:

  • As a first step, we probably want to abort the process if there's more than one required version for the same chart already. This may mean that an upgrade is already in progress OR the environment is manually overridden. By having one item per "require", we have this information in the version check command
  • By having one version check array item per requirement, we can more safely modify the chartfile. We know that the version check item index will match what's in the chartfile

So I'd propose the following format:

[
  {
     name
     description
     directory (etc, all items of the require)
     current_version: { chart_version, app_version }
     latest_version: { chart_version, app_version }
     (if possible) latest_matching_major_version: { chart_version, app_version }
     (if possible) latest_matching_minor_version: { chart_version, app_version }
   }
]

with one item per "require"

I think this gives us more options to work with afterwards. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I definitely like the format to provide more information about each upgrade.

As for exiting the process early if there are multiple versions of the same require, I'm not entirely sure. We have a few chartfile.yaml files that contain multiple charts, and exiting early would mean that we could only be upgrading a single chart at a time. Additionally, unless we had a random choice or something, it could only ever upgrade the first chart in the list if they are releasing faster than we are running our cron. With that in mind, that's why I went with the "just return the info and let the level above figure out what should be upgraded" rather than baking that decision into tanka.

With that in mind, here's a scenario:

# chartfile.yaml
...
requires:
  - chart: registry/chart1
    version: 1.0.0
  - chart: registry/chart1
    version: 1.0.1
  - chart: registry/chart2
    version: 3.0.0

If chart1 latest is 1.0.1, and chart2 latest is 3.0.1, the return I think would be the most amount of information would be

{
  "registry/[email protected]": {"name": "registry/chart1", "latest_version": "1.0.1", ...},
  "registry/[email protected]": {},
  "registry/[email protected]": {"name": "registry/chart2", "latest_version": "3.0.1", ...},
}

Which would allow the higher-level tool for each require to do a quick if {name}@{latest_version} in requires before deciding to do an upgrade.

We would lose out on the list-ordering here by returning the object but I think the output of this will either be used by humans who could add to the requires how they wish, or used by robots where humans no longer have to really manage the chartfile and don't care to maintain the order of the list.

I would have all that info you had in yours in those returned objects, I just cut them short for clarity here.

What are your thoughts about that?

@SimKev2 SimKev2 force-pushed the chart_version_checker branch from c33e07f to c7b4727 Compare May 2, 2024 16:43
@SimKev2
Copy link
Contributor Author

SimKev2 commented May 2, 2024

Running now with example chartfile

# chartfile.yaml
directory: charts
repositories:
- name: fluxcd-community
  url: https://fluxcd-community.github.io/helm-charts
requires:
- chart: fluxcd-community/flux2
  directory: 2.11.0
  version: 2.11.0
- chart: simkev2/nginx-ingress
  directory: 1.1.0
  version: 1.1.0
version: 1

Note that my private repo here only has a version for 1.2.0 of nginx-ingress, and the current latest version of flux2 is 2.12.4.

Without repo config does not get nginx-ingress update :

$ ../../../../../go/src/github.com/grafana/tanka/tk tool charts version-check | jq
{
  "fluxcd-community/[email protected]": {
    "name": "fluxcd-community/flux2",
    "directory": "2.11.0",
    "current_version": "2.11.0",
    "using_latest_version": false,
    "latest_version": {
      "name": "fluxcd-community/flux2",
      "version": "2.12.4",
      "app_version": "2.2.3",
      "description": "A Helm chart for flux2"
    },
    "latest_matching_major_version": {
      "name": "fluxcd-community/flux2",
      "version": "2.12.4",
      "app_version": "2.2.3",
      "description": "A Helm chart for flux2"
    },
    "latest_matching_minor_version": {
      "name": "fluxcd-community/flux2",
      "version": "2.11.1",
      "app_version": "2.1.2",
      "description": "A Helm chart for flux2"
    }
  },
  "simkev2/[email protected]": {
    "name": "simkev2/nginx-ingress",
    "directory": "1.1.0",
    "current_version": "1.1.0",
    "using_latest_version": true,
    "latest_version": {
      "name": "simkev2/nginx-ingress",
      "version": "1.1.0",
      "description": "search did not return 1 version"
    },
    "latest_matching_major_version": {
      "name": "simkev2/nginx-ingress",
      "version": "1.1.0",
      "description": "search did not return 1 version"
    },
    "latest_matching_minor_version": {
      "name": "simkev2/nginx-ingress",
      "version": "1.1.0",
      "description": "search did not return 1 version"
    }
  }
}

With repo config does search private repository as well as the public one :

$ ../../../../../go/src/github.com/grafana/tanka/tk tool charts version-check --repository-config $HOME/Library/Preferences/helm/repositories.yaml | jq
{
  "fluxcd-community/[email protected]": {
    "name": "fluxcd-community/flux2",
    "directory": "2.11.0",
    "current_version": "2.11.0",
    "using_latest_version": false,
    "latest_version": {
      "name": "fluxcd-community/flux2",
      "version": "2.12.4",
      "app_version": "2.2.3",
      "description": "A Helm chart for flux2"
    },
    "latest_matching_major_version": {
      "name": "fluxcd-community/flux2",
      "version": "2.12.4",
      "app_version": "2.2.3",
      "description": "A Helm chart for flux2"
    },
    "latest_matching_minor_version": {
      "name": "fluxcd-community/flux2",
      "version": "2.11.1",
      "app_version": "2.1.2",
      "description": "A Helm chart for flux2"
    }
  },
  "simkev2/[email protected]": {
    "name": "simkev2/nginx-ingress",
    "directory": "1.1.0",
    "current_version": "1.1.0",
    "using_latest_version": false,
    "latest_version": {
      "name": "simkev2/nginx-ingress",
      "version": "1.2.0",
      "app_version": "3.5.0",
      "description": "NGINX Ingress Controller"
    },
    "latest_matching_major_version": {
      "name": "simkev2/nginx-ingress",
      "version": "1.2.0",
      "app_version": "3.5.0",
      "description": "NGINX Ingress Controller"
    },
    "latest_matching_minor_version": {
      "name": "simkev2/nginx-ingress",
      "version": "1.1.0",
      "description": "search did not return 1 version"
    }
  }
}

Copy link
Member

@julienduchesne julienduchesne left a comment

Choose a reason for hiding this comment

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

Great work! A few small comments and then we'll be good to go!

cmd/tk/toolCharts.go Outdated Show resolved Hide resolved
pkg/helm/charts.go Outdated Show resolved Hide resolved
pkg/helm/charts.go Outdated Show resolved Hide resolved
Copy link
Member

@julienduchesne julienduchesne left a comment

Choose a reason for hiding this comment

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

:shipit:

@SimKev2 SimKev2 merged commit 30dac00 into main May 2, 2024
8 checks passed
@SimKev2 SimKev2 deleted the chart_version_checker branch May 2, 2024 18:46
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