asyncapi / spec

The AsyncAPI specification allows you to create machine-readable definitions of your asynchronous APIs.
https://www.asyncapi.com
Apache License 2.0
4.24k stars 269 forks source link

Improve CI workflows in AsyncAPI repositories #423

Closed derberg closed 3 years ago

derberg commented 4 years ago

Context GitHub finally fixed the issue with lack of support for workflows running on forks (tl;dr forks had access to read-only token for security reasons).

Description

  1. first change workflows running on pull_request event to pull_request_target to make sure PRs created from fork have access to write token (it is mostly the welcome workflow like https://github.com/asyncapi/asyncapi/blob/master/.github/workflows/welcome-first-time-contrib.yml (DONE 🟢 thanks to Hacktoberfest)
  2. run tests in workflows not only on linux but windows too (DONE 🟢)
  3. validate titles of PRs with https://github.com/amannn/action-semantic-pull-request to make sure conventional commits are used (DONE 🟢)
  4. Start triggering pull request merge with a specific label. It is a great step to improve the security of your GitHub organization. To be 100% sure we are secure from vulnerabilities (someone left the organization and we forgot to change their access right) and also our own mistakes (accidentally created branch in upstream), no one should have write access to repositories except of admins. Write access should belong only to a bot that can merge pull requests. Rest of the org members can have triage level rights so they can assign issues or add labels. Aside from security improvement it is also good for all org members to easily decide on when to merge a PR by just adding a label
  5. Add twitter workflow (like in html-template and parser) to all repos where we have releases workflows (DONE 🟢)
derberg commented 4 years ago

during SIG meeting @damaru-inc and @jonaslagoni didn't share any concerns with doing automated pull request merge. The best approach seems to be automated merge if we have all approvals and checks and have it "blockable" with a label like do-not-merge.

Important to follow up on who can "Approve a PR". Do you need to be a member of organization or not, cause if not, then we will have to introduce CODEOWNERS first and have the protection that requires approval from a Codeowner

github-actions[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity :sleeping: It will be closed in 30 days if no further activity occurs. To unstale this issue, add a comment with detailed explanation. Thank you for your contributions :heart:

jonaslagoni commented 3 years ago

Would be great if we can start integrating our coverage reports to ensure it does not diminish from PR to PR but stays the same or improve. As @derberg suggested coveralls.io is free for open source projects which can be used to solve this issue.

Based on discussion on Slack

Jonas Lagoni 
Am I the only one who thinks its time we add test coverage as GitHub badge on the repos :face_with_monocle:?

Lukasz Gornicki 
depends what you want to achieve really.
I never look at those. I’m more concerned as maintainer, that tests are added once and then they are not added with new PRs. I liked in one project I contribute to that on PR level there was a check that was making sure my PR is not making coverage worse

Jonas Lagoni 
And that, but as a user I like to see the testing coverage of the library, since it tells you something about the state of it. And since we have it I think we should integrate it :slightly_smiling_face: also we should adapt GitHub workflows to fail if coverage declines. Don't think that's added to any library is it :thinking_face: ? (edited) 

Lukasz Gornicki 
like I said, I never look at this, test coverage is not important for me when I do assessment of an open source project. But I’m not against of course.
and yeap, we do not have any workflow that would asses test coverage and its level on a PR.
the library I referred to in my previous comment was openapi-sampler https://github.com/Redocly/openapi-sampler/pull/117
it uses Coveralls https://coveralls.io/builds/34213343
it is free for open source https://coveralls.io/pricing (edited) 

Jonas Lagoni  
Would be awesome IMO :blobblewobble:

Lukasz Gornicki 
I’m wondering where to add it :thinking_face:
maybe just put a comment with all details here https://github.com/asyncapi/asyncapi/issues/423 so we do not forget? (edited) 

Jonas Lagoni 
Think it's a great place to put it yea :slightly_smiling_face:
derberg commented 3 years ago

Another one to discuss/decide: What slack integration do we use:

problem with semantic release is that it passes release notes to slack rendered and when we have @asyncapi there then a random user on slack is pinged. We could add asyncapi user to slack as a workaround though

Screenshot 2020-12-10 at 14 33 36
derberg commented 3 years ago

we need to switch to latest action for pull request creation as it is failing from time to time, like here https://github.com/asyncapi/markdown-template/runs/1564435740?check_suite_focus=true and causes issues like https://github.com/asyncapi/markdown-template/issues/49. fun fact, action shows error but PR is actually created. We need to switch to 3.0

derberg commented 3 years ago

PR https://github.com/asyncapi/.github/pull/18 addresses => https://github.com/asyncapi/asyncapi/issues/423#issuecomment-742523996 and Add twitter workflow (like in html-template and parser) to all repos where we have releases workflows PR https://github.com/asyncapi/.github/pull/21 addresses => is just about making release workflow easier to manage and more decoupled

derberg commented 3 years ago

for linting of PRs titles I suggest we use https://github.com/amannn/action-semantic-pull-request. It had only one missing feature but I just contributed it last week and it works great now

derberg commented 3 years ago

I'm closing this issue as it grew big with lots of discussions. 2 things in CI from this issue are not done yet. Follow-up issues created: