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

refactor: extract reassure-danger package #118

Merged
merged 17 commits into from
Aug 11, 2022

Conversation

ShaswatPrabhat
Copy link
Contributor

@ShaswatPrabhat ShaswatPrabhat commented Aug 2, 2022

Aim: To extract reassure-danger to a separate package

Link for the issue can be found here

@changeset-bot
Copy link

changeset-bot bot commented Aug 2, 2022

🦋 Changeset detected

Latest commit: 8ff650a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@callstack/reassure-danger Minor
reassure Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ShaswatPrabhat
Copy link
Contributor Author

Above is failing Run danger.js with the error Cannot find module './lib/commonjs/plugin'
I had deleted the plugins file from reassure package for this change. So that dangerFile.ts could pick up directly from the new reassure-danger package.

I think I might have missed something. Please guide.

@mdjastrzebski
Copy link
Member

Danger.js step fails due to following error:
image

Seems like we need to update dangerfile with correct import path.

@mdjastrzebski mdjastrzebski changed the title Feat/reassure danger refactor: extract reassure-danger package Aug 2, 2022
@mdjastrzebski
Copy link
Member

Seems like running yarn turbo run build as part of reassure-tests.sh fixed the issue. But I can no longer see danger output, so were still missing something.

I guess we can also rollback version updates, in favour of changesets.

@ShaswatPrabhat
Copy link
Contributor Author

It is still waiting for one check to complete. Do we still need to make a change to reassure-tests.sh ?

@mdjastrzebski
Copy link
Member

I've exacted the reassure-tests.sh script update and merged it as separate PR to main so that now it's no longer in this PR.

@ShaswatPrabhat
Copy link
Contributor Author

Okay,
For reasure-danger package extraction, are there any other code changes to be done ?

@mdjastrzebski
Copy link
Member

@ShaswatPrabhat For some reason the Danger step is not working properly, as there is no Danger report in this PR. We need to sort this out before merging.

@ShaswatPrabhat
Copy link
Contributor Author

The Run danger.js command gives:

Found only messages, passing those to review. Request failed [403]: https://api.github.com/repos/callstack/reassure/issues/118/comments Response: { Feedback: undefined "message": "Resource not accessible by integration", "documentation_url": "https://docs.github.com/rest/reference/issues#create-an-issue-comment" }

Seems to be a permission issue. But comparing with some other Action runs danger is able to create a comment successfully.

@mdjastrzebski
Copy link
Member

@ShaswatPrabhat yes, this seems to be the error that is blocking us right now. I've checked and previous successful builds, did not have this. The error does not really make sense, maybe there is a "deeper" error that is trigger this error but not build displayed. Is it possible to run Danger on local machine to avoid long CI feedback loop>

@ShaswatPrabhat
Copy link
Contributor Author

ShaswatPrabhat commented Aug 4, 2022

I tried to replicate the issue locally but wasn't able to, Found this issue though. Mine is a PR from fork as well. Hence wondering if that might be the issue. Also found a possible resolution here, but not sure if that is an acceptable one.

@mdjastrzebski mdjastrzebski force-pushed the feat/reassure-danger branch 3 times, most recently from 3af7653 to 48468be Compare August 4, 2022 10:21
@mdjastrzebski mdjastrzebski marked this pull request as draft August 4, 2022 10:21
@mdjastrzebski
Copy link
Member

@ShaswatPrabhat I've found the root cause why do not see the perf report for your PR. It is due to GITHUB_TOKEN not being allowed for forked repos. I've published the same PR but on main repo and there is a correct perf report: #133.

@ShaswatPrabhat
Copy link
Contributor Author

Thank you very much @mdjastrzebski 👍🏼
Seems this will be an issue for most non-core contributors. But I also see we have an open issue to move away from DangerJS fully. Hopefully that would help.

ShaswatPrabhat and others added 7 commits August 8, 2022 11:41
Update dangerfile.ts to use new reassure-danger import path

Remove plugins from reassure package

Update Readme to point to reassure-danger import path

Add changeset for new reassure-danger and minor bump for reassure

chore: attempt to fix danger on CI

chore: attempt 2 to fix danger on CI

Add permissions in main.yaml file for workaround of forked PRs due to dangerjs
@mdjastrzebski
Copy link
Member

mdjastrzebski commented Aug 8, 2022

I am thinking about way the users should import danger plugin:

Option A: import from danger plugin package

// in dangerfile.ts
import reassure from '@callstack/danger-plugin-reassure'

That way we could make root package not include danger-plugin-reassure package, but users would have to add it to their package.json in addition to reassure root package.

Option B: import directly from root package

// in dangerfile.ts
import { dangerReassure } from 'reassure'

Which is good for now, but in the future Danger will not be our only CI integration method.

@thymikee wdyt about these? Maybe you got some other approach?

@mdjastrzebski mdjastrzebski marked this pull request as ready for review August 8, 2022 10:02
@mdjastrzebski mdjastrzebski requested a review from thymikee August 8, 2022 10:02
@mdjastrzebski mdjastrzebski self-requested a review August 8, 2022 14:13
Copy link
Member

@mdjastrzebski mdjastrzebski left a comment

Choose a reason for hiding this comment

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

Looks good. @thymikee please also take a look as I've messed a lot with this PR, so a fresh eye could spot new things.

@thymikee thymikee merged commit 76d6047 into callstack:main Aug 11, 2022
@ShaswatPrabhat ShaswatPrabhat deleted the feat/reassure-danger branch October 7, 2022 07:59
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.

3 participants