chore: update readme #310
chore: update readme #310
Conversation
| @@ -3235,7 +3235,7 @@ module.exports = require("util"); | |||
| /***/ ((module) => { | |||
|
|
|||
| "use strict"; | |||
| module.exports = JSON.parse('{"name":"github-action-merge-dependabot","version":"3.5.0","description":"A GitHub action to automatically merge and approve Dependabot pull requests","main":"src/index.js","scripts":{"build":"ncc build src/index.js","lint":"eslint .","test":"tap test/**.test.js","prepare":"husky install"},"author":{"name":"Salman Mitha","email":"SalmanMitha@gmail.com"},"contributors":["Simone Busoli <simone.busoli@nearform.com>"],"license":"MIT","repository":{"type":"git","url":"git+https://github.com/fastify/github-action-merge-dependabot.git"},"bugs":{"url":"https://github.com/fastify/github-action-merge-dependabot/issues"},"homepage":"https://github.com/fastify/github-action-merge-dependabot#readme","dependencies":{"@actions/core":"^1.9.1","@actions/github":"^5.1.1","actions-toolkit":"github:nearform/actions-toolkit","gitdiff-parser":"^0.2.2","semver":"^7.3.8"},"devDependencies":{"@vercel/ncc":"^0.34.0","eslint":"^8.27.0","eslint-config-prettier":"^8.5.0","eslint-plugin-prettier":"^4.2.1","husky":"^8.0.1","prettier":"^2.7.1","proxyquire":"^2.1.3","sinon":"^14.0.1","tap":"^16.3.0"}}'); | |||
| module.exports = JSON.parse('{"name":"github-action-merge-dependabot","version":"3.5.0","description":"A GitHub action to automatically merge and approve Dependabot pull requests","main":"src/index.js","scripts":{"build":"ncc build src/index.js","lint":"eslint .","test":"tap test/**.test.js","prepare":"husky install"},"author":{"name":"Salman Mitha","email":"SalmanMitha@gmail.com"},"contributors":["Simone Busoli <simone.busoli@nearform.com>"],"license":"MIT","repository":{"type":"git","url":"git+https://github.com/fastify/github-action-merge-dependabot.git"},"bugs":{"url":"https://github.com/fastify/github-action-merge-dependabot/issues"},"homepage":"https://github.com/fastify/github-action-merge-dependabot#readme","dependencies":{"@actions/core":"^1.9.1","@actions/github":"^5.1.1","actions-toolkit":"github:nearform/actions-toolkit","gitdiff-parser":"^0.2.2","semver":"^7.3.8"},"devDependencies":{"@vercel/ncc":"^0.34.0","eslint":"^8.27.0","eslint-config-prettier":"^8.5.0","eslint-plugin-prettier":"^4.2.1","husky":"^8.0.2","prettier":"^2.7.1","proxyquire":"^2.1.3","sinon":"^14.0.2","tap":"^16.3.1"}}'); | |||
There was a problem hiding this comment.
this change is unexpected. not necessarily wrong though. can you check what's the diff with the original? to be safe, I'd exclude it
README.md
Outdated
| on: | ||
| push: | ||
| branches: | ||
| - master |
There was a problem hiding this comment.
main instead of master.
Or place both.
Actually, it is better to create a separate one instead of updating Basic Example.
There was a problem hiding this comment.
Hey @climba03003, thanks for pointing this out. I will update the branch.
Leaving Basic Example as it is may be misleading as the actions used in the composite one will fail if the filter on push is not properly set.
There was a problem hiding this comment.
Basic Example means to be basic usage. Not everyone need to filter out the push event and I don't see the needs to filter out by default.
There was a problem hiding this comment.
@climba03003 it is necessary because this action doesn't work on pushes, so filtering is necessary and should be in the basic example
There was a problem hiding this comment.
If the requirement of workflow is Don't run on PULL, then it should add a if clause.
The changes here is NOT don't run on pull, but only run on master | main branch when it is pull.
if: github.event_name == 'pull_request'
There was a problem hiding this comment.
We intentionally avoided the if on the step to keep the usage of the action simple. A CI workflow anyway is better configured the way it is here, because if you don't filter the push trigger you'll always have duplicate runs, which is not what you usually want.
There was a problem hiding this comment.
Anyway, I don't block it. But it's weird for the reason.
If you want to keep it simple, then no exception. And not everyone wish to prevent the push trigger.
It even doesn't hurt if you run multiple times (ensure no flaky test).
There was a problem hiding this comment.
I see your point, we used to have that in the past. Now for another couple of reasons it may make sense to reintroduce this condition on the step itself, but I think we can handle it even better within the action itself, which we aren't at the moment.
There was a problem hiding this comment.
@simoneb
I see the issue on your end and the update on readme should not solve your issue. It will throw error even with the filter because it runs on pull event.
The proper fix should be update the action.yml, so dependabot/fetch-metadata will runs on PR only. It is the one causing problem.
github-action-merge-dependabot/action.yml
Line 42 in 484a2cd
Closes #311
Changes include: