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

Generate chart README.md with helm-docs #674

Merged
merged 2 commits into from
Oct 21, 2021
Merged

Generate chart README.md with helm-docs #674

merged 2 commits into from
Oct 21, 2021

Conversation

k0da
Copy link
Collaborator

@k0da k0da commented Oct 20, 2021

Extend chart values comments and use them as description for helm-docs
generated README

Fixes #359

Signed-off-by: Dinar Valeev dinar.valeev@absa.africa

Makefile Outdated Show resolved Hide resolved
ytsarev
ytsarev previously approved these changes Oct 20, 2021
Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

looks indeed cool. Candidate for the pipeline to automatically keep docs in sync?

@k0da
Copy link
Collaborator Author

k0da commented Oct 21, 2021

This is extended now to do it in a PR fassion. Once chart changes are commited to master, PR will be opened with updated Helm Docs. No user intervention needed.

@k0da k0da force-pushed the helmDocs branch 2 times, most recently from 0f250b3 to c7d37b8 Compare October 21, 2021 10:25
.github/workflows/helm_docs.yaml Outdated Show resolved Hide resolved
interval: "20s"
securityContext: # For more options consult https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.22/#securitycontext-v1-core
fsGroup: 65534 # For ExternalDNS to be able to read Kubernetes and AWS token files
# -- For more options consult https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.22/#securitycontext-v1-core
Copy link
Member

@jkremser jkremser Oct 21, 2021

Choose a reason for hiding this comment

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

it would be really cool if the helm-docs tool supported some variables and we could parametrize this URL, because they will be stale soon and actually they sunset the docs for older versions of k8s. It's not related to your change, though.

chart/k8gb/values.yaml Outdated Show resolved Hide resolved
@k0da k0da requested a review from jkremser October 21, 2021 11:05
@k0da k0da force-pushed the helmDocs branch 2 times, most recently from ca17218 to eff9ae6 Compare October 21, 2021 11:16
@k0da
Copy link
Collaborator Author

k0da commented Oct 21, 2021

looks indeed cool. Candidate for the pipeline to automatically keep docs in sync?

Done

chart/k8gb/values.yaml Outdated Show resolved Hide resolved
- name: Create Pull Request
uses: peter-evans/create-pull-request@v3
with:
title: "Generate Helm Docs"
Copy link
Contributor

Choose a reason for hiding this comment

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

@k0da cool GHA :)
Just a small wording suggestion:

Suggested change
title: "Generate Helm Docs"
title: "Update Helm Docs"

"Generate" feels like it might be generated but not necessarily updated in the end.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be cool if we can refer to the "originator" PR in the title or description, but I'm not sure how we can do this at this stage (push to master).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In theory we can, but this blows scope of PR a lot. Lets start small first

Copy link
Member

Choose a reason for hiding this comment

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

there is that

      - name: Check PR
        run: |
          echo "Pull Request Number - ${{ steps.cpr.outputs.pull-request-number }}"
          echo "Pull Request URL - ${{ steps.cpr.outputs.pull-request-url }}"

where cpr is the id of that pull request step. It's not title, though. It relies on the output of the step so I guess, it's not even possible to have it in the title (chicken-egg)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, something like that, but we need to find it in one of the steps. This will be run on push to master. PR scope is lost at this point

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

@k0da , do we keep the target than ?

PHONY: helm-docs
helm-docs:
	helm-docs -c chart/k8gb

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can drop it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kuritka done

Copy link
Member

Choose a reason for hiding this comment

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

@jkremser we don't have PR#, it doesn't run as PR job. we have to find commits witch changes charts/k8gb/* and find last PR. Since this is mostly cosmetics, I'd do that after "main" action is in place already.

That's what the step, I've posted, actually does :) The peter-evans/create-pull-request@v3 action exposes the PR url and PR number implicitly (docs), no need to do it manually by finding commits/prs.

PR scope is lost at this point

that's why the id reference

here is the full version:

.github/workflows/helm_docs.yaml
name: Helm docs
on:
  push:
    branches:
    - 'master'
    paths:
     - 'chart/k8gb/**'
jobs:
  build-helm-doc:
    name: Update Helm Doc
    runs-on: ubuntu-latest
    steps:
    - name: Checkout Code
      uses: actions/checkout@v2
    - name: Check all charts README.md
      uses: docker://jnorwood/helm-docs:v1.5.0
    - name: Create Pull Request
      id: foo
      uses: peter-evans/create-pull-request@v3
      with:
        title: "Update Helm Docs"
        branch: ci-helm-doc
        delete-branch: true
        base: master
        signoff: true
        token: ${{ secrets.GITHUB_TOKEN }}
     - name: Echo PR info
       run: |
          echo "Pull Request Number - ${{ steps.foo.outputs.pull-request-number }}"
          echo "Pull Request URL - ${{ steps.foo.outputs.pull-request-url }}"

btw. no need to do that in this PR, lgtm, good stuff

@k0da k0da force-pushed the helmDocs branch 2 times, most recently from 97acbcc to c8bc8f2 Compare October 21, 2021 12:22
jkremser
jkremser previously approved these changes Oct 21, 2021
k0da added 2 commits October 21, 2021 17:05
Extend chart values comments and use them as description for helm-docs
generated README

Fixes #359

Signed-off-by: Dinar Valeev <dinar.valeev@absa.africa>
Signed-off-by: Dinar Valeev <dinar.valeev@absa.africa>
Copy link
Collaborator

@kuritka kuritka left a comment

Choose a reason for hiding this comment

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

I'm looking forward to seeing it 👍

@k0da k0da merged commit 94c2047 into master Oct 21, 2021
@k0da k0da deleted the helmDocs branch October 21, 2021 17:55
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.

Create separate README.md file for k8gb helm chart
5 participants