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
Split into Action, App, CLI, Core and Switch to TypeScript #1204
base: master
Are you sure you want to change the base?
Conversation
A boolean indicating whether the releaser mode is disabled. | ||
required: false | ||
default: '' | ||
disable-autolabeler: |
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.
I intend to create a separate action for autolabeler
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.
Some small suggestions, great work! 👏 🎉
dist/ | ||
node_modules/ | ||
lib/ |
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.
lib/ | |
lib/ | |
Missing EOL
@@ -26,22 +26,26 @@ jobs: | |||
run: | | |||
gh pr checkout ${{ github.event.pull_request.number }} | |||
|
|||
- uses: pnpm/action-setup@v2 |
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.
Out of curiosity, what are you reason(s) to switch to pnpm
?
Asking as it may be interesting to use it too for (plugins.)jenkins.io
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.
pnpm solves a problem with monorepo shared dependencies are hard links or symlinks vs yarn and npm duplicating the storage.
Also the fact that I can share dev dependencies across workspaces
if [ -z "$(git status --porcelain)" ]; then | ||
echo "💾 no changes to dist/index.js" | ||
if [ "$(git diff --ignore-space-at-eol | wc -l)" -eq "0" ]; then | ||
echo "❎ no changes" |
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.
I would use ℹ️ instead of a (green) cross which would make me think twice before deciding if it's all good.
exit 0 | ||
fi | ||
git add . |
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.
git add . | |
git add dist/index.js |
or change the commit and echo lines to say it might have added more than just this file.
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.
No, I do not want to try and figure out which dist it is.
Cause this will host multiple actions.
So it would be ./packages/action/dist
or ./packages/autolabeler-action/dist
or even more with the planned ./packages/pr-label-checker/dist/
reason for ./packages/autolabeler-action
is that ./packages/app
wants to reuse autolabeler so a ./packages/autolabeler
is needed.
git fetch origin master | ||
git push https://heroku:$HEROKU_API_TOKEN@git.heroku.com/$HEROKU_APP_NAME.git origin/master:master --force | ||
git fetch origin main | ||
git push https://heroku:$HEROKU_API_TOKEN@git.heroku.com/$HEROKU_APP_NAME.git origin/main:master --force |
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.
git push https://heroku:$HEROKU_API_TOKEN@git.heroku.com/$HEROKU_APP_NAME.git origin/main:master --force | |
git push https://heroku:$HEROKU_API_TOKEN@git.heroku.com/$HEROKU_APP_NAME.git origin/main:main --force |
(not sure about this one)
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.
I don't want to change it on heroku just yet, sounds scary to run heroku repo:reset -a APP-NAME
run: .github/no-unstaged-files.sh | ||
run: | | ||
if [ "$(git diff --ignore-space-at-eol | wc -l)" -gt "0" ]; then | ||
echo "::error::💥 Unstaged changes detected. Locally try running: pnpm prettier:fix && pnpm lint:fix && pnpm build" |
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.
Shouldn't the instructions order here be the same as the instructions above?
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.
Order doesn't matter between prettier and lint, prettier corrects other files such as yaml and markdown as well :)
ESlint is configured for prettier so order does not matter.
}, | ||
) | ||
|
||
// const drafter = async (context) => { |
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.
Commented code block, to be deleted?
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.
I am slowly implementing it. It is still in draft.
packages/core/src/releases.ts
Outdated
const lastRelease = | ||
sortedPublishedReleases[sortedPublishedReleases.length - 1] | ||
|
||
// TODO(jetersen): Move to outer methods |
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.
Comment to be updated?
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.
The todo can be removed it is done.
However I plan to refactor the code some more.
I think I might have to deal with logging in core but in a better way.
I don't want to use Probot in ./packages/app
because it has too many deps and cannot update fast enough.
They are still stuck on Pino v6. Also the app only uses webhooks, so octokit webhooks should suffice.
Also I don't want to use Pino, it adds too much coding over head imho.
GitHub action has core.info
and core.warning
which maps to process.stdout
I want to explore the refactoring options.
this alllows for easier transformation of string to regex!
use prettier to format schema.json
Co-authored-by: Hervé Le Meur <91831478+lemeurherve@users.noreply.github.com>
This is mostly an entire rewrite of the code base into typescript with multiple packages.
I will rewrite history a lot in the pull request and it is definitely not in a working state at the moment.
I may reduce the code changes required so that I can get this into the main branch. Split some of the newer features into multiple pull requests to benefit from pull requests as release notes 👏
Also as a way to get feedback on v6 while it lives in main and being released in prereleases.
Reduces set:
pnpm
package manager dependabot/dependabot-core#1736pnpm
package manager dependabot/dependabot-core#1736fixes #549
fixes #667
fixes #148
fixes #1146