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

chore: Lint eco system error #4275

Merged
merged 8 commits into from Sep 20, 2022
Merged

chore: Lint eco system error #4275

merged 8 commits into from Sep 20, 2022

Conversation

zrosenbauer
Copy link
Contributor

@zrosenbauer zrosenbauer commented Sep 14, 2022

Overview

Added annotations + summary output for the Linting GHA. See examples of outputs below:

Summary Sample

Screen Shot 2022-09-18 at 7 03 39 PM

Annotations Sample

Screen Shot 2022-09-18 at 7 04 06 PM

Checklist

@zrosenbauer
Copy link
Contributor Author

@Eomm working on #4275 & need CI to test, can you approve?

I also am not sure a comment is the best idea as its super noisy. Probably better to update the built output (it can take markdown) or annotate the codes (I can do that also I believe as I've done with Terraform repos).

@nooreldeensalah
Copy link
Contributor

nooreldeensalah commented Sep 14, 2022

If you wish to test the workflow file locally, you can use https://github.com/nektos/act (But this might be irrelevant to #4265)

After installing it, you can run it using
act -W .\.github\workflows\lint-ecosystem-order.yml -s GITHUB_TOKEN=<github_access_token>

@Eomm
Copy link
Member

Eomm commented Sep 14, 2022

I also am not sure a comment is the best idea as its super noisy. Probably better to update the built output (it can take markdown) or annotate the codes (I can do that also I believe as I've done with Terraform repos).

Users do not look at the CI output, so a comment (only for failures) or the code annotation would be great!

@zrosenbauer
Copy link
Contributor Author

@Eomm if my code aint' broke this should work, and is there anyway to not have to re-run this (I've used act but just not the same, especially when leaving comments) 😄

@jsumners
Copy link
Member

An annotation would be ideal. Bot comments can be really noisy.

@zrosenbauer
Copy link
Contributor Author

@jsumners we can do annotations if you think thats the best route (I re-pushed with the comment version for now).

The issue with the bot comments is every push builds new one (maybe) dealt with on TF repos where each new tf plan was a new comments (45 comments later...)

@jsumners
Copy link
Member

The issue with the bot comments is every push builds new one (maybe) dealt with on TF repos where each new tf plan was a new comments (45 comments later...)

I don't know what this means.

@zrosenbauer
Copy link
Contributor Author

The issue with the bot comments is every push builds new one (maybe) dealt with on TF repos where each new tf plan was a new comments (45 comments later...)

I don't know what this means.

tl:dr I overshared... bot comments are super noisey. I'll switch to annotations if I can get some time this weekend.

The reason I know bot comments are noisey....

I experienced this first hand with an action I wrote for "terraform" aka every terraform plan output was appended to the PR. We had PRs with 40+ bot comments 😢 (I turned it off and had it triggered via a label).

@zrosenbauer zrosenbauer marked this pull request as ready for review September 19, 2022 00:14
@zrosenbauer
Copy link
Contributor Author

@Eomm @jsumners updated and ready to go IMO, let me know if you need other changes / tweaks.

@zrosenbauer zrosenbauer requested review from jsumners and Eomm and removed request for jsumners and Eomm September 19, 2022 14:18
@zrosenbauer zrosenbauer changed the title Lint eco system error chore: Lint eco system error Sep 19, 2022
Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

The script lgtm

image

Not sure the about the changes on the GHA

@@ -1,12 +1,6 @@
name: Lint Ecosystem Order

on:
push:
Copy link
Member

Choose a reason for hiding this comment

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

Whi this has been changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't clear on the intent, as the previous version should run on every branch push not named main / master which would mean it will run outside of a PR and 2x on a PR (push + pull_request). In the current version it only runs on PRs (which I believe is the desired state).

In addition, the script will only work on PRs. If we want to support PRs and not PRs, I need to tweak the script to run differently depending on the context & we should probably have 2 workflows.

I also wouldn't understand if its not running on main/master why we'd ever believe its not running on a PR....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsumners hey what was intent behind push and pull_request trigger?

Copy link
Member

Choose a reason for hiding this comment

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

I think in past, che pull_request event was not triggered when new commits where pushed to the PR's branch.

Thanks for the clarification

I think it is good with this lighter setup

cc/ @Fdawgs

@@ -1,12 +1,6 @@
name: Lint Ecosystem Order

on:
push:
Copy link
Member

Choose a reason for hiding this comment

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

I think in past, che pull_request event was not triggered when new commits where pushed to the PR's branch.

Thanks for the clarification

I think it is good with this lighter setup

cc/ @Fdawgs

@Fdawgs Fdawgs linked an issue Sep 20, 2022 that may be closed by this pull request
Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@Fdawgs Fdawgs merged commit 21eb6cf into fastify:main Sep 20, 2022
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ecosystem lint check post comment
5 participants