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 wireit #353

Merged
merged 2 commits into from
Jan 15, 2024
Merged

Conversation

quentinderoubaix
Copy link
Contributor

No description provided.

Copy link

codecov bot commented Jan 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (45aec83) 92.22% compared to head (b0dadcf) 91.18%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #353      +/-   ##
==========================================
- Coverage   92.22%   91.18%   -1.05%     
==========================================
  Files          95       95              
  Lines        2109     2109              
  Branches      384      384              
==========================================
- Hits         1945     1923      -22     
- Misses         93      109      +16     
- Partials       71       77       +6     
Flag Coverage Δ
e2e-1 36.50% <ø> (ø)
e2e-2 60.63% <ø> (ø)
e2e-3 62.87% <ø> (-0.29%) ⬇️
e2e-4 53.20% <ø> (ø)
e2e-6 ?
e2e-8 81.64% <ø> (ø)
e2e-9 58.07% <ø> (ø)
unit 90.45% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@divdavem
Copy link
Member

divdavem commented Jan 9, 2024

Thank you for this PR. wireit looks very interesting.
I am thinking that if a task is managed by wireit (especially for caching), we should always pass through wireit to execute it to best benefit from the cache. That's not the case currently, especially for generate:exports that is run right from the start (in the prepare script). This means we should avoid having "npm run ..." in wireit commands (because that means we can do the same npm run ... without passing through wireit).
What do you think? Maybe we should integrate this PR and then improve that later?

Also, this build worries me a bit because it failed, and re-running it succeeded. Did you investigate why? Is there any race condition when wireit runs? Does wireit run things in parallel and in some scenario, could it lead to some errors? I find this strange.

@quentinderoubaix
Copy link
Contributor Author

quentinderoubaix commented Jan 11, 2024

Thank you for this PR. wireit looks very interesting. I am thinking that if a task is managed by wireit (especially for caching), we should always pass through wireit to execute it to best benefit from the cache. That's not the case currently, especially for generate:exports that is run right from the start (in the prepare script). This means we should avoid having "npm run ..." in wireit commands (because that means we can do the same npm run ... without passing through wireit). What do you think? Maybe we should integrate this PR and then improve that later?

Also, this build worries me a bit because it failed, and re-running it succeeded. Did you investigate why? Is there any race condition when wireit runs? Does wireit run things in parallel and in some scenario, could it lead to some errors? I find this strange.

thx for your initial review @divdavem !

I have updated the PR to wire wireit everywhere that seemed interesting.
Investigating the build that failed, it seemed linked to how the cache is managed with the wire github action.
For the moment, I propose to merge this PR without enabling the github cache, and we would enable it with another PR.

What do you think ?

Copy link
Member

@divdavem divdavem left a comment

Choose a reason for hiding this comment

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

Thank you for your PR! I think it can finally be merged now.

@quentinderoubaix
Copy link
Contributor Author

thx for your review @divdavem !

@quentinderoubaix quentinderoubaix merged commit 1b764c3 into AmadeusITGroup:main Jan 15, 2024
12 of 13 checks passed
@quentinderoubaix quentinderoubaix deleted the writeit branch January 15, 2024 15:58
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.

2 participants