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

Don't call yarn directly in jenkins-jobs. #259

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

jeresig
Copy link
Member

@jeresig jeresig commented Jan 22, 2025

Summary:

This is a follow-up to:
https://github.com/Khan/webapp/pull/28829

I changed the few places where we install packages to use make npm_deps instead. The two places where we were calling a yarn script I now use the new shell script for this, instead. This is all in service of moving off of yarn v1 to pnpm v10 instead.

Issue: FEI-6143

Test plan:

Will need to deploy the webapp changes first. But I'm a bit unclear on how to best test this!

@jeresig jeresig requested a review from a team as a code owner January 22, 2025 16:32
@jeresig jeresig self-assigned this Jan 22, 2025
@jeresig jeresig requested a review from a team January 22, 2025 16:32
Copy link
Contributor

@kevinb-khan kevinb-khan left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

Copy link
Member

@csilvers csilvers left a comment

Choose a reason for hiding this comment

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

It would be great if you updated the commit message to make it clearer that the reason for doing this is to enable an eventual transition away from yarn.

Assuming that's the motivation, this looks good to me! I knew make would win the day in the end. :-)

Will need to deploy the webapp changes first. But I'm a bit unclear on how to best test this!

The easiest way is once your webapp changes are in master. Then you can pick the latest run for each of these jobs and hit replay, e.g. https://jenkins.khanacademy.org/job/deploy/job/e2e-test/70455/replay/. Then replace the entire "main script" section with your new code (as of this PR), and run it.

It looks like demo-district and e2e-test-cycloud are not currently passing, so I don't know that you can test them. For webapp-maintenance, I think it's fine to just wait until the weekend and see how it does; it's not the end of the world if it fails. But you can also do the "replay" thing for that if you'd like.

@csilvers
Copy link
Member

If you want to test before your webapp changes are deployed, you can do that too. It'll take two steps. First, you run the job (say, e2e-test) on the branch you want to test; that is, you set GIT_REVISION to point to your branch. Once that's started -- you can let it run to completion, or abort it -- you can select that job and hit "replay" and put in your new code.

@@ -40,13 +40,13 @@ BUILD_NAME = "build demo-district-password-update";
def _setupWebapp() {
kaGit.safeSyncToOrigin("[email protected]:Khan/webapp", params.CYPRESS_GIT_REVISION);
dir("webapp/services/static") {
sh("yarn install");
sh("make npm_deps");
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, John, for fixing it!

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.

4 participants