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

Split into Action, App, CLI, Core and Switch to TypeScript #1204

Draft
wants to merge 81 commits into
base: master
Choose a base branch
from

Conversation

jetersen
Copy link
Member

@jetersen jetersen commented Aug 20, 2022

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:


fixes #549
fixes #667
fixes #148
fixes #1146

A boolean indicating whether the releaser mode is disabled.
required: false
default: ''
disable-autolabeler:
Copy link
Member Author

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

Copy link

@lemeurherve lemeurherve left a 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/

Choose a reason for hiding this comment

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

Suggested change
lib/
lib/

Missing EOL

@@ -26,22 +26,26 @@ jobs:
run: |
gh pr checkout ${{ github.event.pull_request.number }}

- uses: pnpm/action-setup@v2

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

Copy link
Member Author

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"

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 .

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Member Author

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

Choose a reason for hiding this comment

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

Suggested change
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)

Copy link
Member Author

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"

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?

Copy link
Member Author

@jetersen jetersen Aug 29, 2022

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) => {

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?

Copy link
Member Author

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 Show resolved Hide resolved
const lastRelease =
sortedPublishedReleases[sortedPublishedReleases.length - 1]

// TODO(jetersen): Move to outer methods

Choose a reason for hiding this comment

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

Comment to be updated?

Copy link
Member Author

@jetersen jetersen Aug 29, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants