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

feat(setup): add github actions as CI option #502

Merged
merged 23 commits into from
Dec 20, 2019

Conversation

jeetiss
Copy link
Contributor

@jeetiss jeetiss commented Dec 4, 2019

closes #346
closes #489

@jeetiss
Copy link
Contributor Author

jeetiss commented Dec 8, 2019

I added base and manual config for github actions, but for some reasons, when shipjs prepare runs on github, conventional-changelog detect only latest commit message

I ran from my computer 👉🏻https://github.com/jeetiss/try-shipjs/pull/22/files
And everything okey

This result I saw when trigger ship prepare from action 👉🏻https://github.com/jeetiss/try-shipjs/pull/22/files
Wrong next version because оnly last commit message detected

@uetchy @eunjae-lee any ideas why this happening?

@jeetiss jeetiss marked this pull request as ready for review December 8, 2019 10:34
@eunjae-lee
Copy link
Contributor

Ah... this might be because of the recent change I made.
9841f25#diff-aa9168540654167618719f74a15159eaR22
Now it creates a temp file to pass config to conventional-changelog in order to specify the revision range.
We probably need to drop conventional-changelog-cli and instead use its node package directly.
Let me know if this is the case. I will try to work on this issue.

@eunjae-lee
Copy link
Contributor

I just merged #525.
If you update your branch, the problem hopefully will be solved.

@jeetiss
Copy link
Contributor Author

jeetiss commented Dec 12, 2019

Thanks 🙌🏻
I am finish working on this PR on weekend

@jeetiss
Copy link
Contributor Author

jeetiss commented Dec 14, 2019

@eunjae-lee, I realize, that I can't test #525, and I hope it'll be work!

PR ready for review, I test prepare command with yarn workspace shipjs test:run:setup --dir and Github Actions on my try-shipjs repo!

Last thing that we should discuss about, it is GITHUB_TOKEN

Github Actions have this token on default, but it don't have enough access permissions for push tags and create release, I can't create another token with that name, because of that I choose another name for token 👉🏻 GH_TOKEN

Maybe we should use that name for CircleCI and Github Actions for consistency? Or will be enough just point to it in readme? Ow, readme! I need help with readme!

@eunjae-lee
Copy link
Contributor

eunjae-lee commented Dec 16, 2019

Hi @jeetiss 👋🏼,
Thanks for the work!

Was the reason you couldn't test #525 because it wasn't released?

And about GH_TOKEN, when user sets up like it,

GITHUB_TOKEN: \${{ secrets.GH_TOKEN }}

will Ship.js get process.env.GITHUB_TOKEN or process.env.GH_TOKEN?

@jeetiss
Copy link
Contributor Author

jeetiss commented Dec 16, 2019

Was the reason you couldn't test #525 because it wasn't released?

Yes.

And about GH_TOKEN

Ship.js can get process.env.GITHUB_TOKEN

but for it user need setup GH_TOKEN in https://github.com/@username/@reponame/settings/secrets/

Снимок экрана 2019-12-16 в 14 42 56

Copy link
Contributor

@eunjae-lee eunjae-lee left a comment

Choose a reason for hiding this comment

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

Thanks so much for a lot of effort on this PR 🎉
I left some comments.
Let me know how you think.

And I guess we can go with GH_TOKEN and write a proper guide why we decided to do so, instead of forcing all existing users to move on to new env var.
Normally they will use either CircleCI or GitHub Actions. So they will have either GITHUB_TOKEN or GH_TOKEN, but not both of them, right? What do you think?

@jeetiss
Copy link
Contributor Author

jeetiss commented Dec 17, 2019

@eunjae-lee I find why prepare fails in github actions, its my fault. actions/checkout on default fetch only 1 commit from history! see fetch-depth param
https://github.com/actions/checkout#usage

and now it should works fine!

@eunjae-lee
Copy link
Contributor

What would be the best way for me to test this PR?
Releasing Ship.js beta version and run it on a repo with GitHub Actions?

@jeetiss
Copy link
Contributor Author

jeetiss commented Dec 17, 2019

You can run yarn workspace shipjs test:run:setup --dir ~/workspace/your-test-repository , push it to github and test actions without beta release 😉

@eunjae-lee
Copy link
Contributor

@jeetiss
Copy link
Contributor Author

jeetiss commented Dec 18, 2019

It is private, I see 404

@jeetiss
Copy link
Contributor Author

jeetiss commented Dec 18, 2019

I find problem 🤪
My redactor adds some tabs when I copy-paste configs
Nice catch

@eunjae-lee
Copy link
Contributor

eunjae-lee commented Dec 18, 2019

Sorry it was private. Now it's public.

I updated the ymls and now I get another error.

https://github.com/eunjae-lee/dummy-lib-1111/commit/9457ec7e93d491bfaf8f5724bf58c94927d487de/checks?check_suite_id=365582870

Can you check it please? I think the error comes from here: https://github.com/algolia/shipjs/blob/master/packages/shipjs-lib/src/lib/git/isWorkingTreeClean.js#L5:L5

And when you see this issue https://github.com/eunjae-lee/dummy-lib-1111/issues/87 ,
the comment is like

image

and it'd be nice if we get rid of the quote around the comment 🙂

Sorry for not responding to you quickly enough.

@jeetiss
Copy link
Contributor Author

jeetiss commented Dec 19, 2019

I don't add support for yarn yet, so action fails with it

npm install creates package-lock.json
shipjs prepare fails with - The working tree is not clean.

https://github.com/eunjae-lee/dummy-lib-1111/commit/9457ec7e93d491bfaf8f5724bf58c94927d487de/checks?check_suite_id=365582870#step:4:1

I can detect that yarn using in repo and change install command in actions or just run npx shipjs prepare
First one need some code, but will run faster. What do you think?

Fix that

Sorry for not responding to you quickly enough.

I don't hurry, lets make better solution 🙌🏻

@eunjae-lee
Copy link
Contributor

@eunjae-lee
Copy link
Contributor

@jeetiss I guess we can merge this now. What do you think? 🙂

@jeetiss
Copy link
Contributor Author

jeetiss commented Dec 20, 2019

Yes, I test all things
And its work with npm too ^_^
jeetiss/try-shipjs#30

Thanks for so simple solution

@eunjae-lee
Copy link
Contributor

Thanks for the amazing work 🎉

Copy link
Contributor

@eunjae-lee eunjae-lee left a comment

Choose a reason for hiding this comment

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

I ran linter and tests on my local, so I'm merging this without waiting for CircleCI,
because they don't run on the PRs from external contributors.

@eunjae-lee eunjae-lee merged commit 923cc87 into algolia:master Dec 20, 2019
@jeetiss jeetiss deleted the feat-github-actions branch December 28, 2019 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add config for GitHub actions on "setup" stage Invoke shipjs prepare from GitHub
2 participants