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

[PDE-4679] fix(cli): better handling of broken symlinks while copying files to temp directory during build process #737

Merged
merged 5 commits into from
Feb 1, 2024

Conversation

rnegron
Copy link
Member

@rnegron rnegron commented Jan 29, 2024

TO-DO: This is still missing some tests.

Intended to fix #706. Modifies the copyDir method by using lstat to avoid crashing when reading a faulty symbolic link.

About lstat from the manual pages:

Screenshot 2024-01-30 at 11 25 44 AM

Example of new warning message:

Screenshot 2024-01-31 at 4 33 22 PM

@rnegron rnegron requested a review from kreddlear January 29, 2024 21:13
@rnegron rnegron requested a review from eliangcs as a code owner January 29, 2024 21:13
Copy link
Member

@eliangcs eliangcs left a comment

Choose a reason for hiding this comment

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

zapier build produces two files:

  • build/build.zip
  • build/source.zip.

source.zip already respects .gitignore. However, I don't think build.zip should exclude the files in .gitignore. When running zapier build --skip-npm-install, we intentionally don't run npm install internally. Instead, it copies the files from the working directory's node_modules. So even though node_modules is in .gitignore, we should include it in build.zip.

I believe the problem with #706 is not about .gitignore but is about invalid symlinks. zapier build should show a better error message when it encounters a bad symlink. Let me know if I missed anything though!

Also, be sure to test different options:

  • zapier build --skip-npm-install (currently crashing with this PR)
  • zapier build --disable-dependency-detection

@rnegron
Copy link
Member Author

rnegron commented Jan 30, 2024

@eliangcs

So even though node_modules is in .gitignore, we should include it in build.zip.

OK, this makes sense to me! I was wondering if just using gitignore was the correct approach or not. I will re-tool this PR to add some logic for detecting an invalid symlink and erroring appropriately.

@rnegron rnegron changed the title [PDE-4679] fix(cli): check .gitignore before copying files to temp directory during build process [PDE-4679] fix(cli): better handling of broken symlinks while copying files to temp directory during build process Jan 31, 2024
@rnegron rnegron changed the base branch from main to PDE-4678-build-in-workspaces January 31, 2024 20:28
@rnegron
Copy link
Member Author

rnegron commented Jan 31, 2024

Hmm, using lstat causes the describe('build in workspaces') tests to fail. Checking...

Update: the issue is not lstat but rather the fact that yarn workspaces creates symlinks to directories such as: /private/var/folders/28/81v0tq5s2cv9zcnvhgtrr9jw0000gn/T/zapier-90e1abb9/node_modules/app-1 that point to ../packages/app-1, and symlinks-to-directories are not supported yet. Will fix tomorrow.

@rnegron
Copy link
Member Author

rnegron commented Jan 31, 2024

I was thinking that, in order to address the original issue, simply alerting the user that the problem was a broken symlink might be enough and we don't need to get too creative by differentiating between file or directories...

The question is then: should we skip the faulty symlink or error out of the build process?

@rnegron rnegron marked this pull request as ready for review January 31, 2024 22:28
@eliangcs
Copy link
Member

eliangcs commented Feb 1, 2024

The question is then: should we skip the faulty symlink or error out of the build process?

I think skipping with a very visible warning (like you're doing now) would be good.

This looks good to me. Your comment reminds me that we need to skip the symlinks that point to a workspace. I'll go ahead and merge this, so I can fix that in my PR. Thanks!

@eliangcs eliangcs merged commit f9687d7 into PDE-4678-build-in-workspaces Feb 1, 2024
13 checks passed
@eliangcs eliangcs deleted the PDE-4679 branch February 1, 2024 07:18
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.

[Bug]: zapier build follows invalid symlinks when copying
2 participants