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 CI via GitHub Actions #80

Merged
merged 3 commits into from
Jun 17, 2022
Merged

Add CI via GitHub Actions #80

merged 3 commits into from
Jun 17, 2022

Conversation

colearendt
Copy link
Contributor

@colearendt colearendt commented May 27, 2022

What this PR does / why we need it:

Add CI:

  • lint/test
    • lint changed charts
    • then lint all charts
    • run kind
      (these are allowed to fail)
    • install changed charts
    • install all charts
  • release (on main, see if any new chart versions - if so, GitHub release and update the index.yaml on the gh-pages branch)
  • rebuild (rebuilds the index.yaml file from GitHub release history)

Some open items to discuss here:

  • how do we do this in such a way that it does not affect your current setup? Should we create a gh-pages branch to maintain the "new" index.yaml on until you want to cut over the definition of GitHub pages? Will that affect anything?
  • should we remove the "changed vs. all" components of the action? It is most useful when there is more than one chart in a repo. Which side should we err on? Only run lint/test when there are changes? Always run them (even on docs PRs?)
  • are any of the dependencies concerning? Of note:
    • azure/setup-helm
    • actions/checkout
    • peter-evans/create-pull-request
    • helm/chart-releaser-action, helm/chart-testing-action, helm/kind-action
  • we also have an action that we use to code-generate README.md (to keep documentation up to date with the values.yaml file) via helm-docs. Is this something that would be interesting to you?
  • are there any arguments to linting and testing that would be helpful? i.e. --upgrade maybe? The goal of separating here was to allow preventing installation if lint fails
    docker_exec ct lint-and-install --charts couchdb --upgrade --chart-dirs .

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

Special notes for your reviewer:

Checklist

(removed: NA)

@willholley
Copy link
Member

willholley commented Jun 17, 2022

Thanks for the PR @colearendt - it generally looks good to me. On the discussion points:

how do we do this in such a way that it does not affect your current setup? Should we create a gh-pages branch to maintain the "new" index.yaml on until you want to cut over the definition of GitHub pages? Will that affect anything?

I think this is right, and should be a safe change for existing users:

should we remove the "changed vs. all" components of the action?

If the change detection is reliable then I'm happy to go with linting changes only

are any of the dependencies concerning?

They all look well used/maintained, so no concerns from me.

we also have an action that we use to code-generate README.md (to keep documentation up to date with the values.yaml file) via helm-docs. Is this something that would be interesting to you?

Yes - that sounds helpful!

are there any arguments to linting and testing that would be helpful? i.e. --upgrade maybe?

I think testing with --upgrade makes sense as we do today. This has caught a few issues for us in the past.

fix a typo in inline command
add --upgrade flag to install checks
switch `ct lint` to run less frequently (changed on branches, all on main)
@colearendt
Copy link
Contributor Author

colearendt commented Jun 17, 2022

TIL about .asf.yaml! Nice!

I have made the discussed changes:

  • run lint-only changes on branches (still running all on main because there are never any changes otherwise. Testing/linting on main still seems desirable). It is pretty simple to revert this at some point, and is low cost/fast anyways if you did want to lint always. One benefit of running based on changed/always is consistency with the install checks
  • Add --upgrade flag to ct install tests

One last idea:

  • wanted to make sure that push to main and pull_request is the right definition for triggers. i.e. pushing to a random branch will not trigger CI unless a PR is created. IIRC one of the problems of just using "push to any branch" was that it did not trigger CI for remote PRs, and having both "all branches" and "PR"s would trigger duplicate CI runs on PRs.

@willholley
Copy link
Member

run lint-only changes on branches (still running all on main because there are never any changes otherwise. Testing/linting on main still seems desirable).

It seems like if we do this we should also require linear history (we can set in .asf.yaml)

wanted to make sure that push to main and pull_request is the right definition for triggers

That seems right to me.

+1

@willholley willholley merged commit 1781b0c into apache:main Jun 17, 2022
@willholley willholley mentioned this pull request Jun 17, 2022
@colearendt colearendt deleted the add-ci branch June 18, 2022 11:04
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.

Run checks for chart version metadata update as GH Action
2 participants