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

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

Merged
merged 4 commits into from
May 29, 2024

Conversation

jessebot
Copy link
Collaborator

Pull Request

Description of the change

Only run the chart linting action for:

  • charts/nextcloud/Chart.yaml
  • charts/nextcloud/values.yaml
  • charts/nextcloud/templates/** (everything in the templates directory)

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, #574, is trying to run the chart linter 🤦

Checklist

@jessebot jessebot self-assigned this May 29, 2024
@provokateurin
Copy link
Member

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

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.

@jessebot
Copy link
Collaborator Author

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 :)

@jessebot jessebot requested a review from provokateurin May 29, 2024 13:16
@provokateurin
Copy link
Member

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
Copy link
Member

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
Copy link
Collaborator Author

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? 🤔

.github/workflows/lint-test.yaml Outdated Show resolved Hide resolved
.github/workflows/lint-test.yaml Show resolved Hide resolved
@provokateurin
Copy link
Member

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.

@jessebot jessebot requested a review from provokateurin May 29, 2024 13:43
@jessebot jessebot merged commit ba6ce9d into nextcloud:main May 29, 2024
3 checks passed
@jessebot jessebot deleted the update-chart-linting branch May 29, 2024 13:58
SwitzerChees pushed a commit to SwitzerChees/helm that referenced this pull request Jun 27, 2024
…every exception (nextcloud#575)

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

Signed-off-by: jessebot <[email protected]>

* attempt to adapt changes from https://github.com/nextcloud/.github/blob/master/workflow-templates/node.yml

Signed-off-by: jessebot <[email protected]>

* add needs: changes to lint job

Signed-off-by: jessebot <[email protected]>

* remove summary job afterall

Signed-off-by: jessebot <[email protected]>

---------

Signed-off-by: jessebot <[email protected]>
Signed-off-by: switzerchees <[email protected]>
raynay-r pushed a commit to raynay-r/nextcloud-helm that referenced this pull request Jun 28, 2024
…every exception (nextcloud#575)

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

Signed-off-by: jessebot <[email protected]>

* attempt to adapt changes from https://github.com/nextcloud/.github/blob/master/workflow-templates/node.yml

Signed-off-by: jessebot <[email protected]>

* add needs: changes to lint job

Signed-off-by: jessebot <[email protected]>

* remove summary job afterall

Signed-off-by: jessebot <[email protected]>

---------

Signed-off-by: jessebot <[email protected]>
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.

2 participants