-
-
Notifications
You must be signed in to change notification settings - Fork 246
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
ci: Enhance GitHub Actions workflows #1370
Conversation
okineadev
commented
Jan 28, 2025
- Added descriptive step names and emojis in various workflows to enhance readability
- Ensured consistent formatting across all workflows for a unified look
* Added descriptive step names and emojis in various workflows to enhance readability * Ensured consistent formatting across all workflows for a unified look
✅ Deploy Preview for creative-fairy-df92c4 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not much of a fan of having emojis in actions personally but changes look fine otherwise.
@aklinker1 has to decide on this though |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, I like some of the changes, dislike some others.
- I would also prefer not to have emojis.
- I prefer to not name my jobs/steps to keep things as minimal as possible... But since WXT has grown more popular and there are more contributors, maybe it makes sense to give everything names so people can quickly understand what's going on. I'm onboard with giving names to everything
- I disagree with some of your whitespace choices... But I'll push a commit getting rid of the ones I don't agree with.
permissions: | ||
contents: read | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whats the point of adding this? I don't think we use permissions on any of the other workflows
permissions: | |
contents: read |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whats the point of adding this? I don't think we use permissions on any of the other workflows
Security.
It is necessary to grant as few rights as possible (only necessary ones), not all of them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that the default though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that the default though?
I'll check it out in a bit, I'm currently removing the emoji from the titles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that the default though?
Co-authored-by: Aaron <aaronklinker1@gmail.com>
+1 for enforcing least privilege. It's more secure to explicitly grant the permissions needed by a job/workflow than to rely on the repo's defaults (and hope they never accidentally or maliciously get changed in a way that expands permissions for jobs that don't need them) +1 for naming things to improve clarity for new contributors (although I'm fine with no emojis) I would recommend also adding a CODEOWNERS file that highly restricts who can approve changes to the |
You could also configure CodeQL to detect vulnerabilities, I did this in the organization and my projects earlier |
Yes, this is an important aspect, especially in checks for pull requests, as malicious code can be inserted there |
Regarding emojis - it really helps me navigate workflows, but if there are too many of them, it will look too childish in the logs, it's purely a matter of taste |
There are use cases with emojis in the @material-extensions organization |
No I can't |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I've opened a PR into your fork with my suggested changes: okineadev-forks#1
@wxt-dev/auto-icons
@wxt-dev/module-solid
@wxt-dev/i18n
@wxt-dev/module-svelte
@wxt-dev/module-react
@wxt-dev/module-vue
@wxt-dev/storage
@wxt-dev/unocss
wxt
commit: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1370 +/- ##
==========================================
- Coverage 81.55% 81.12% -0.44%
==========================================
Files 128 128
Lines 6296 6284 -12
Branches 1072 1069 -3
==========================================
- Hits 5135 5098 -37
- Misses 1146 1171 +25
Partials 15 15 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, sorry that took longer than usual, but it's good to go now!