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

Fix Yarn Crowdin scripts #11082

Merged
merged 2 commits into from
Feb 15, 2021
Merged

Fix Yarn Crowdin scripts #11082

merged 2 commits into from
Feb 15, 2021

Conversation

slorber
Copy link
Contributor

@slorber slorber commented Feb 12, 2021

Summary

@SimenB @merceyz the change made here:
jest-website-migration@fd34cd0

It broke Crowdin translation download (didn't notice)
Crowdin is quite sensitive to the path from which you call its cli, and it seems using the : and global yarn script does not run from the place where the cli is declared, unlike yarn workspace.

image

image

Test plan

Netlify preview works

@codecov-io
Copy link

Codecov Report

Merging #11082 (cebd69d) into jest-website-v2 (2fd0ce1) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff                @@
##           jest-website-v2   #11082   +/-   ##
================================================
  Coverage            64.22%   64.22%           
================================================
  Files                  305      305           
  Lines                13255    13255           
  Branches              3235     3235           
================================================
  Hits                  8513     8513           
  Misses                4050     4050           
  Partials               692      692           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fd0ce1...cebd69d. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Feb 12, 2021

Some $pwd thing? interesting. Would love @arcanis and @merceyz 👀 on this

@slorber
Copy link
Contributor Author

slorber commented Feb 12, 2021

$pwd ?

the crowdin cli should be run from the root so I think we can use yarn workspace or yarn .. (as --cwd apparently does not work?)

@SimenB
Copy link
Member

SimenB commented Feb 12, 2021

Right, I meant cwd (my thought process was cwd is "run pwd in a terminal" which prints cwd, thus the confusion).

My question is - why is the working directory different between yarn workspace @jest/monorepo crowdin:upload and yarn workspace crowdin:upload if any scripts with a : in it is supposed to be global?

@arcanis
Copy link
Contributor

arcanis commented Feb 12, 2021

I can double check tomorrow, but I'm fairly sure it's not the case - colon-scripts and workspace <script> should have the same effect 🤔

@slorber slorber changed the title Restore working Crowdin scripts Fix Yarn Crowdin scripts Feb 15, 2021
@slorber
Copy link
Contributor Author

slorber commented Feb 15, 2021

You are right @arcanis my mistake, I had the crowdin:upload script declared in both the monorepo + subpackage 😅

Should be better now, ready to review again :)

@netlify
Copy link

netlify bot commented Feb 15, 2021

Deploy preview for jestjs ready!

Built with commit 3e9cc5a

https://deploy-preview-11082--jestjs.netlify.app

@SimenB SimenB merged commit eb34210 into jestjs:jest-website-v2 Feb 15, 2021
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants