nextcloud / helm

A community maintained helm chart for deploying Nextcloud on Kubernetes.
GNU Affero General Public License v3.0
295 stars 258 forks source link

only run chart linting for specific files instead of trying to catch every exception #575

Closed jessebot closed 1 month ago

jessebot commented 1 month ago

Pull Request

Description of the change

Only run the chart linting action for:

Benefits

This is better than trying to manually exclude all the possible things we don't want to run it on.

Possible drawbacks

Can't think of any.

Additional information

This would fix the issue where this PR, https://github.com/nextcloud/helm/pull/574, is trying to run the chart linter 🤦

Checklist

provokateurin commented 1 month ago

Hm now the PR doesn't pass, because the workflow is set to be required. Could you adapt the workflow so that there is a summary job that always like in https://github.com/nextcloud/.github/blob/master/workflow-templates/node.yml? Then we can mark that one as required and we won't accidentally merge with red CI.

jessebot commented 1 month ago

Hm now the PR doesn't pass, because the workflow is set to be required. Could you adapt the workflow so that there is a summary job that always like in https://github.com/nextcloud/.github/blob/master/workflow-templates/node.yml? Then we can mark that one as required and we won't accidentally merge with red CI.

lemme give it a try!

jessebot commented 1 month ago

oh, that did something! :O You're a genius, Kate!

jessebot commented 1 month ago

Hm this looks wrong, as the chart testing step should be skipped since the changes in this PR didn't touch any of those files.

You're right, I missed the needs parameter for the lint-test job. The final summary job is currently called Lint and Test Charts, which is an attempt to match the workflow you linked where the workflow itself is called node and the final summary job is also node. Do you want to keep that same formatting? Open to thoughts and opinions :)

provokateurin commented 1 month ago

You can just change the name if you like. I'll change which workflow is required afterwards (but it will require every PR to be rebased).

provokateurin commented 1 month ago

Looks like it is also fine to have the required job being skipped? Then we don't even need the summary job which was probably was just added for compatibility.

jessebot commented 1 month ago

You can just change the name if you like. I'll change which workflow is required afterwards (but it will require every PR to be rebased).

oof, rebasing every PR in this repo will be pain.

Looks like it is also fine to have the required job being skipped? Then we don't even need the summary job which was probably was just added for compatibility.

I'm confused. How do I do that? 🤔

provokateurin commented 1 month ago

oof, rebasing every PR in this repo will be pain.

I think we actually don't need that, as the name of the required job won't change.