Skip to content

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

Merged
jessebot merged 4 commits into
nextcloud:mainfrom
jessebot:update-chart-linting
May 29, 2024
Merged

only run chart linting for specific files instead of trying to catch every exception#575
jessebot merged 4 commits into
nextcloud:mainfrom
jessebot:update-chart-linting

Conversation

@jessebot

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

…every exception
Signed-off-by: jessebot <jessebot@linux.com>
@jessebotjessebot self-assigned this May 29, 2024
@provokateurin

Copy link
Copy Markdown
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
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
CollaboratorAuthor

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

@provokateurinprovokateurin left a comment

Copy link
Copy Markdown
Member

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.

Signed-off-by: jessebot <jessebot@linux.com>
@jessebot

Copy link
Copy Markdown
CollaboratorAuthor

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
CollaboratorAuthor

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

Comment thread.github/workflows/lint-test.yaml Outdated
Comment thread.github/workflows/lint-test.yaml
@provokateurin

Copy link
Copy Markdown
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.

Signed-off-by: jessebot <jessebot@linux.com>
@jessebot
jessebot merged commit ba6ce9d into nextcloud:mainMay 29, 2024
@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 <jessebot@linux.com>
* attempt to adapt changes from https://github.com/nextcloud/.github/blob/master/workflow-templates/node.yml
Signed-off-by: jessebot <jessebot@linux.com>
* add needs: changes to lint job
Signed-off-by: jessebot <jessebot@linux.com>
* remove summary job afterall
Signed-off-by: jessebot <jessebot@linux.com>
---------
Signed-off-by: jessebot <jessebot@linux.com>
Signed-off-by: switzerchees <patrickmichel96@yahoo.de>
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 <jessebot@linux.com>
* attempt to adapt changes from https://github.com/nextcloud/.github/blob/master/workflow-templates/node.yml
Signed-off-by: jessebot <jessebot@linux.com>
* add needs: changes to lint job
Signed-off-by: jessebot <jessebot@linux.com>
* remove summary job afterall
Signed-off-by: jessebot <jessebot@linux.com>
---------
Signed-off-by: jessebot <jessebot@linux.com>
Sign up for freeto 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

@jessebot@provokateurin