-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
account-compression: Move sdk to pnpm workspace #5910
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good! Just 1 qq on the CI stuff
@@ -8,6 +8,7 @@ on: | |||
- "ci/*-version.sh" | |||
- "ci/install-anchor.sh" | |||
- ".github/workflows/pull-request-account-compression.yml" | |||
- "!account-compression/sdk/**" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this do & why add?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The !
means to not run if there's a change in one of those files.
The idea is to not run this workflow if there's only a change to the JS side, since pull-request-js.yml
gets run if there's only a change on the JS side. But if there's a change in the program, you do want to run this workflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if there's a change in the SDK the tests should run. SDK can change test behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, the SDK tests are run by pull-request-js.yml
if there's a change there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining!
Problem
During the update to 1.17.6 in #5863, it looks like the build barfs on account-compression not being part of the pnpm workspace
Solution
Move the account-compression sdk to the same workspace! This way it'll also pick up dependabot changes like the rest of the repo.
This also required some dependency bumps because the current build has some unmet dependencies according to pnpm.
Note that the linting isn't working, so I can fix that in a follow-up PR.