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

Linter using Github Actions #8

Closed
wants to merge 13 commits into from

Conversation

SarveshLimaye
Copy link

#3

Added super-linter using github actions

Copy link
Contributor

@dduportal dduportal left a comment

Choose a reason for hiding this comment

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

Hello @SarveshLimaye thanks for this PR.

  • The current workflow succeeded, but did not executed any lint on the helm charts provided in the repository. Usually, the tool ct (https://github.com/helm/chart-testing) is used for linting helm charts
  • Whatever tool is used, could you add a documentation to explain for a contributor how they could execute the same linting on their own machine when contributing? The idea is that we should not wait for a PR with the GitHub workflow execution to be able to lint.

@SarveshLimaye
Copy link
Author

@dduportal Thanks for the feedback. :)
The documentation for linting has to be added in readme right? Can I attach the link to the official doc of the super-linter? It is explained in detail there.

@lemeurherve
Copy link
Member

* The current workflow succeeded, but did not executed any lint on the helm charts provided in the repository. Usually, the tool `ct` ([helm/chart-testing](https://github.com/helm/chart-testing)) is used for linting helm charts

Official GHA: https://github.com/helm/chart-testing-action

@SarveshLimaye
Copy link
Author

* The current workflow succeeded, but did not executed any lint on the helm charts provided in the repository. Usually, the tool `ct` ([helm/chart-testing](https://github.com/helm/chart-testing)) is used for linting helm charts

Official GHA: https://github.com/helm/chart-testing-action

@lemeurherve Thanks for this . It should be exactly same as the example workflow in official gha right ?

@lemeurherve
Copy link
Member

lemeurherve commented Nov 4, 2021

It should be quite similar, ot sure if it should be exactly the same.

@SarveshLimaye
Copy link
Author

@lemeurherve @dduportal is it fine now ? I am sorry by mistakenly I opened a new pull request. I have closed it now.

@dduportal
Copy link
Contributor

Hello @SarveshLimaye , thanks for your work! The check linter is failing with the following error message:

Run changed=$(ct list-changed)
  changed=$(ct list-changed)
  if [[ -n "$changed" ]]; then
    echo "::set-output name=changed::true"
  fi
  shell: /usr/bin/bash -e {0}
  env:
    pythonLocation: /opt/hostedtoolcache/Python/3.7.12/x64
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.7.12/x64/lib
    CT_CONFIG_DIR: /opt/hostedtoolcache/ct/v3.4.0/x86_64/etc
    VIRTUAL_ENV: /opt/hostedtoolcache/ct/v3.4.0/x86_64/venv
Error: Error running process: exit status 128
Error: Process completed with exit code 1.

I'm not sure why to be honest... We'll take time to review next week, but if you know what to do with this error, please proceed

@SarveshLimaye
Copy link
Author

Hello, I am not sure about the error. Can you pls review the changes.

@lemeurherve
Copy link
Member

@SarveshLimaye sorry for the late response, I think the problem comes from here: helm/chart-testing#330
I'll add a suggestion to try fixing it.

@lemeurherve
Copy link
Member

Tried to remove the shallow cloning, it didn't worked...

@lemeurherve
Copy link
Member

Related issue: helm/chart-testing-action#69

Copy link
Member

@lemeurherve lemeurherve left a comment

Choose a reason for hiding this comment

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

This should fix the check, I've made a PR in the action repository too: helm/chart-testing-action#75

- name: Run chart-testing (list-changed)
id: list-changed
run: |
changed=$(ct list-changed)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
changed=$(ct list-changed)
changed=$(ct list-changed --target-branch main)

@SarveshLimaye
Copy link
Author

SarveshLimaye commented Dec 8, 2021

This should fix the check, I've made a PR in the action repository too: helm/chart-testing-action#75

@lemeurherve Thank you so much for the fix . Is it fine now ?

@SarveshLimaye
Copy link
Author

SarveshLimaye commented Dec 8, 2021

Don't know why the test is still failing.

@dduportal
Copy link
Contributor

Hi @SarveshLimaye ! Many thanks for your patience and efforts to make this happen!

I'm trying to reproduce the issue loclly but I got a bad feeling about ct.
I don't really understand why the "latest" release is 2.5.0 (on both the GitHub repository and homebrew on my macOS workstation), while the github action uses the 3.4.0.

Currently, the github actions reports the following error when running the command ct lint (with ct v3.4.0):

Linting charts...
------------------------------------------------------------------------------------------------------------------------
Error: Error linting charts: Error identifying charts to process: Error running process: exit status 128
No chart changes detected.
------------------------------------------------------------------------------------------------------------------------
Error linting charts: Error identifying charts to process: Error running process: exit status 128

If I try to reproduce on my current local setup (ct 2.5.0) I also got an error:

ct lint
Linting charts...
Error loading configuration: 'chart_schema.yaml' neither specified nor found in default locations

=> I assume that we should target 3.4.0. The goal for you and I is to reproduce the error locally before going forward.
=> Another task for you: do not forget to rebase your PR against the latest version:

  • Add a git remote named upstream on your working directory, that point to the upstream (e.g. git remote add upstream https://github.com/jenkins-infra/helm-charts/, then check with git remote -v that the setup is ok. Once OK, or if already done, retrieve the latest changes with git fetch --all --prune)
    ** Then run the git rebase upstream/main command, followed by git push origin main --force if the rebase was successful.
    ** OR if you prefer merging, you can git merge upstream/main and git push origin main

@dduportal
Copy link
Contributor

@SarveshLimaye Another tip: https://github.com/helm/chart-testing/blob/main/doc/ct_lint.md#synopsis .
The command ct lint only check for changed charts (which is an empty list by default). You need more flags to ensure that it does lint all the available charts.

@SarveshLimaye
Copy link
Author

Hey @dduportal Thanks for the review and tips. Will try to reproduce the error locally for 3.4.0.

@dduportal
Copy link
Contributor

Hello 👋

As the jenkins-infra team is trying to move as much workload to Jenkins controller as possible, we are closing this issue.

@dduportal dduportal closed this Aug 4, 2022
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.

3 participants