helm icon indicating copy to clipboard operation
helm copied to clipboard

Update PR template to not include bumping the chart version

Open provokateurin opened this issue 3 years ago • 5 comments

Pull Request

Description of the change

Removes the requirement for bumping the Chart version in PRs.

Benefits

Collecting multiple PRs before making a new release and also avoid rebasing PRs so often.

Possible drawbacks

None

Applicable issues

Additional information

Discussed in the NC Containers public chat with @tvories

Checklist

provokateurin avatar Dec 10 '22 12:12 provokateurin

This will remove the requirement to set the chart version, but will that prevent the workflow from trying to release when a PR is merged?

tvories avatar Dec 12 '22 17:12 tvories

Oh I haven't though of that. Maybe let's only run the release workflow on tags, then we can just tag the commit that bumps the version?

provokateurin avatar Dec 12 '22 17:12 provokateurin

Would that even work? AFAICT chart-releaser creates the tag and the github release already

provokateurin avatar Dec 12 '22 17:12 provokateurin

I am sure there is a modification to our workflow that we can do to allow us to only publish a chart release on a tag. I think that's all built into the way we currently have it set up, though, so I am thinking we will need to revamp the whole process before we can remove the chart bump requirement

tvories avatar Dec 12 '22 18:12 tvories

I think we can change this:

https://github.com/nextcloud/helm/blob/a491977458b5111cc0afd06386aa63ef46720f4c/.github/workflows/release.yaml#L4-L11

to just be:

on:
  push:
    tags:
      - "*.*.*"

as the release job that runs is just doing:

  • document workflow
  • fetch history
  • install helm
  • add chart dependencies
  • run chart releaser

I don't think anything else relies on this, because linting will still run on PRs. This would actually help the other issue #339, however there is one thing I need to test before I can say for sure it's good to go:

In the chart release github action they check for changes since latest tag here:

Code block for checking for chart changes since latest tag
        echo 'Looking up latest tag...'
        local latest_tag
        latest_tag=$(lookup_latest_tag)

        echo "Discovering changed charts since '$latest_tag'..."
        local changed_charts=()
        readarray -t changed_charts <<< "$(lookup_changed_charts "$latest_tag")"

        if [[ -n "${changed_charts[*]}" ]]; then
            install_chart_releaser

            rm -rf .cr-release-packages
            mkdir -p .cr-release-packages

            rm -rf .cr-index
            mkdir -p .cr-index

            for chart in "${changed_charts[@]}"; do
                if [[ -d "$chart" ]]; then
                    package_chart "$chart"
                else
                    echo "Chart '$chart' no longer exists in repo. Skipping it..."
                fi
            done

            release_charts
            update_index
        else
            echo "Nothing to do. No chart changes detected."
        fi

If it runs on new tags, and the tag it pulls is always the latest tag, it will always exit instead of finishing up it's tasks, including packaging the chart 🤦. If that's the case though, the solution there is to pass in skip-packaging, which skips that bit and continues onto the rest, but then we need to run our own packaging step, so in their code they're doing cr package "$chart" --package-path .cr-release-packages which is the equivilent of helm package --destination .cr-release-packages, so, if in my testing I see that on tag doesn't work, we could also in the following changes to release.yaml:

      # ref: https://helm.sh/docs/helm/helm_package/#helm-package
      # (destination is set for chart-releaser job step below to know where to check)
      - name: Package chart
        run: |
          helm package --destination .cr-release-packages

      - name: Run chart-releaser
        uses: helm/[email protected]
        with:
          charts_repo_url: https://nextcloud.github.io/helm
          # ref: https://github.com/helm/chart-releaser-action#inputs
          skip_packaging: true
        env:
          CR_TOKEN: "${{ secrets.GITHUB_TOKEN }}"

Let me test on a fork first though. Sorry for the info dump. It was just a lot of context 😅

jessebot avatar Jan 26 '23 15:01 jessebot