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: use correct publishDir when building from CLI with cwd option #1264

Merged
merged 6 commits into from
Mar 21, 2022
Merged

Conversation

lourd
Copy link

@lourd lourd commented Mar 15, 2022

Summary

When building using the Netlify CLI with the cwd option, the PUBLISH_DIR is an absolute path
debugger with cwd

Without the cwd option, it is a relative path
debugger without cwd

If it's an absolute path, it's a bug to join it on the cwd, this ends up injecting a path into the function that leads outside of the project. This change checks for if it's an absolute path before joining it.

I also changed the value logged when the build cannot be found to print the full path instead of the relative path from the cwd; while debugging this, the value printing out for me was just "../.." which wasn't helpful.

Test plan

I've tested against the reproduction I made here https://github.com/lourd/netlify-cli-repro/blob/main/package.json#L9 and it works. I'm not sure how to make an automated demo test using the CLI.

Relevant links (GitHub issues, Notion docs, etc.) or a picture of cute animal

Closes #1258.

Screen Shot 2022-03-15 at 1 05 14 AM

Standard checks:

  • Check the Deploy Preview's Demo site for your PR's functionality
  • Add docs when necessary

🧪 Once merged, make sure to update the version if needed and that it was published correctly.

@netlify
Copy link

netlify bot commented Mar 15, 2022

🔮 Deploy Preview for netlify-plugin-nextjs-demo canceled.

Name Link
🔨 Latest commit 86652a7
🔍 Latest deploy log https://app.netlify.com/sites/netlify-plugin-nextjs-demo/deploys/62302f979e0b600009c92494

@netlify
Copy link

netlify bot commented Mar 15, 2022

Deploy Preview for netlify-plugin-nextjs-nx-monorepo-demo ready!

Name Link
🔨 Latest commit 7917442
🔍 Latest deploy log https://app.netlify.com/sites/netlify-plugin-nextjs-nx-monorepo-demo/deploys/62386c710d73010009a5cf26
😎 Deploy Preview https://deploy-preview-1264--netlify-plugin-nextjs-nx-monorepo-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Mar 15, 2022

Deploy Preview for netlify-plugin-nextjs-export-demo ready!

Name Link
🔨 Latest commit 7917442
🔍 Latest deploy log https://app.netlify.com/sites/netlify-plugin-nextjs-export-demo/deploys/62386c719c45ef0009e0935e
😎 Deploy Preview https://deploy-preview-1264--netlify-plugin-nextjs-export-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Mar 15, 2022

👷 Deploy request for netlify-plugin-nextjs-static-root-demo pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 7917442

Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

Thanks for this! I think it could be simplified a little.

@lourd lourd requested a review from ascorbic March 15, 2022 14:04
ascorbic
ascorbic previously approved these changes Mar 16, 2022
@ascorbic ascorbic changed the title Use correct publishDir when building from CLI with cwd option fix: use correct publishDir when building from CLI with cwd option Mar 16, 2022
@ascorbic ascorbic added the type: bug code to address defects in shipped code label Mar 16, 2022
ascorbic
ascorbic previously approved these changes Mar 16, 2022
@lourd
Copy link
Author

lourd commented Mar 17, 2022

Anything else to do here for merging? Wasn't sure with the failing checks and warning about the branch being out of date

@tiffafoo
Copy link

The failing e2e tests are expected! We just realized that secrets don't get shared to forks.

@lourd
Copy link
Author

lourd commented Mar 17, 2022

Will them failing keep the automerge from working?

@kodiakhq
Copy link
Contributor

kodiakhq bot commented Mar 21, 2022

This PR currently has a merge conflict. Please resolve this and then re-add the automerge label.

@kodiakhq kodiakhq bot removed the automerge label Mar 21, 2022
@kodiakhq kodiakhq bot merged commit e441c97 into opennextjs:main Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge type: bug code to address defects in shipped code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: publishDir incorrect when deploying using Netlify CLI with cwd option
3 participants