ci: Optimize CI processes

docsifyjs / docsify

🃏 A magical documentation site generator.

https://docsify.js.org

MIT License

27.47k stars 5.67k forks source link

ci: Optimize CI processes #2451

Closed sy-records closed 3 months ago

sy-records commented 3 months ago

close #2452 close #2453

vercel[bot] commented 3 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
docsify-preview	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 10, 2024 1:27pm

Koooooo-7 commented 3 months ago

Could you don't remove the update-emoji.yaml and update it instead? Then we can manually check the workflow in PR branches in Action since it already in develop.

sy-records commented 3 months ago

you can see https://github.com/sy-records/docsify/actions/runs/9442492589/job/26004732688

Koooooo-7 commented 3 months ago

you can see https://github.com/sy-records/docsify/actions/runs/9442492589/job/26004732688

But I can not trigger it on that place.

I wanna simply test it here with multi manually runs to check the behavior （the PRs behavior, the branch） before we merge it. e.g. It auto triggered on today, and we don't merge it yet. The next auto run (manually run) will create a new PR (and close the pre one) or do nothing? At a glance, it looks fine to me. But I wanna double check it here and make sure it works as expectations.

sy-records commented 3 months ago

You need to fork and test in your own repo, add a push event, remove the if: github.repository == 'docsifyjs/docsify', and modify reviewer to yourself.

Please test in your own repo since testing that will initiate an invalid PR.

Koooooo-7 commented 3 months ago

You need to fork and test in your own repo, add a push event, remove the if: github.repository == 'docsifyjs/docsify', and modify reviewer to yourself.

I see the if: github.repository == 'docsifyjs/docsify' can prevent trigger on forks. What I mean is we can just test here as a real situation.

Please test in your own repo since testing that will initiate an invalid PR.

I think raise an invalid PR is fine to me since we won't merge it.

Koooooo-7 commented 3 months ago

Hi @sy-records , Plz take a look on https://github.com/docsifyjs/docsify/actions/runs/9447704932/job/26020091254 . （It's not in a fork） It seems the PR config for Reviewers not work.

sy-records commented 3 months ago

good catch, The default token does not have org:read permissions and requires the use of PAT.

Koooooo-7 commented 3 months ago

On the test here https://github.com/docsifyjs/docsify/actions/runs/9448830558/job/26023692530 This action is updated to only works on a single branch sync-emoji, so we can not create multi PRs for it. Problems:

We can not manually trigger the action before the branch related PR closed.
It is not be a daily sync job since it can not run success before we merge the first PR if it exists. i.e. It run in the 2024-06-10 and raise a PR. If we merge it in 2024-06-15, the days between 06-10~06-15's jobs won't run success. And we are only able to check the changes in the sync day 2024-06-10. What the changes update after 2024-06-10, it only could be found on the next job run.

If we could have a change to always review/deal with the latest changes PR on the review day, it would be better.

sy-records commented 3 months ago

There is no need for multiple branches and multiple PRs. The new commit will be forced to be pushed at runtime, so there is no need to create a new PR repeatedly.

Koooooo-7 commented 3 months ago

There is no need for multiple branches and multiple PRs. The new commit will be forced to be pushed at runtime, so there is no need to create a new PR repeatedly.

I thought the behavior same to you mentioned that the opened PR should up to date, but the action is failed tho. Otherwise, it looks good to me.