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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(command-deploy): align file filtering with buildbot #1272

Merged
merged 2 commits into from Sep 23, 2020

Conversation

erezrokah
Copy link
Contributor

@erezrokah erezrokah commented Sep 23, 2020

- Summary

Related to netlify/js-client#157 and #1227.

Fixes #1225 (comment)

The CLI deploy command relies on js-client default filter to skip some files when deploying.
This is done here:
https://github.com/netlify/js-client/blob/55650048fc7fe6288816c1dc172b27024cbd1586/src/deploy/util.js#L8

and trying to emulate the code here:
https://github.com/netlify/open-api/blob/e72e2eb2d7eedf02bccc5916fc0330022f7f823b/go/porcelain/deploy.go#L692

This PR fixes a few discrepancies with our buildbot:

  1. Allows publishing a hidden directory (e.g. .public) - fixes part of Manual deployment hangs with a deployment folder that starts with a dot聽#1227.
  2. Allows deploying node_modules when inside a publish folder. Fixes Community report: CLI behaviour does not match containerized behaviour regarding .gitignore files聽#1225 (comment) and matches buildbot behaviour. Only difference is that node_modules are ignored if a user tries to publish the current project dir.
  3. Splits the regular expression here https://github.com/netlify/js-client/blob/55650048fc7fe6288816c1dc172b27024cbd1586/src/deploy/util.js#L13 into string operations. Also I believe the \/__MACOSX part wasn't working as I'm not sure how basename can result in a string containing a /.

- Test plan

Added tests

- Description for the changelog

Align deploy file filtering with buildbot

- A picture of a cute animal (not mandatory but encouraged)

馃惔

let actualContent
try {
const response = await fetch(`${siteUrl}${path}`)
if (response.ok) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the test always fail when response.ok is not true? If so, I am wondering if there should be a t.true(response.ok)?
Also wondering about whether the test should always fail when fetch() or response.text() throws. In which case, t.notThrowsAsync() could be used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, nevermind, I see that content is sometimes undefined in some of the tests. 馃憤

let actualContent
try {
const response = await fetch(`${siteUrl}${path}`)
if (response.ok) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, nevermind, I see that content is sometimes undefined in some of the tests. 馃憤

@erezrokah
Copy link
Contributor Author

Tests are failing since the team we're using to deploy edge handlers on CI doesn't have access to do so anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug code to address defects in shipped code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Community report: CLI behaviour does not match containerized behaviour regarding .gitignore files
2 participants