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

Manual deployment hangs with a deployment folder that starts with a dot #1227

Closed
d4h0 opened this issue Sep 12, 2020 · 20 comments · Fixed by #1274
Closed

Manual deployment hangs with a deployment folder that starts with a dot #1227

d4h0 opened this issue Sep 12, 2020 · 20 comments · Fixed by #1274
Assignees
Labels
area: command: deploy type: bug code to address defects in shipped code

Comments

@d4h0
Copy link

d4h0 commented Sep 12, 2020

Describe the bug

Executing 'netlify deploy -d .build' does not find any files, and hangs during the upload.

Here is the output of a tried deployment:

Deploying to draft URL...
✔ Finished hashing 0 files
✔ CDN requesting 0 files
✔ Finished uploading 0 assets
⧇ Waiting for deploy to go live...

You see, no files were found (even so there are files).

To Reproduce

Steps to reproduce the behavior:

  1. Setup a Netlify project
  2. on Linux, create a hidden directory (a directory that starts with a dot, i.e. ".deploy")
  3. Put some HTML files into the hidden directory
  4. Start the deployment of the hidden folder

Configuration

  • If possible, please copy/paste below your netlify.toml.

I'm using the default netlify.toml (I didn't change anything).

  • Please enter the following command in a terminal and copy/paste its output:
  System:
    OS: Linux 5.8 Arch Linux
    CPU: (8) x64 Intel(R) Core(TM) i7-4710MQ CPU @ 2.50GHz
    Memory: 1.96 GB / 15.52 GB
    Container: Yes
    Shell: 5.0.18 - /bin/bash
  Binaries:
    Node: 14.10.1 - /usr/bin/node
    npm: 6.14.8 - /usr/bin/npm
    Watchman: 4.9.0 - /usr/bin/watchman
  npmGlobalPackages:
    netlify-cli: 2.61.2

Expected behavior

The CLI finds files in the deployment folder, and the upload succeeds (at least, if there is a problem, there should be an error message)

CLI Output

See above

Additional context

After changing the name of the deployment folder from ".build" to "build", everything works as expected

@d4h0 d4h0 added the type: bug code to address defects in shipped code label Sep 12, 2020
@monsonjeremy
Copy link
Contributor

@erezrokah Any advice on how this should be handled?
Here's the discovery I did:

It looks like this is the desired behavior, here the hashFiles function is called using the given dir
https://github.com/netlify/js-client/blob/4cb1f240bac527305ff98032a801accf22dea5bf/src/deploy/index.js#L49

The hashFiles function uses the defaultFilter which ignores hidden files/directories
https://github.com/netlify/js-client/blob/4cb1f240bac527305ff98032a801accf22dea5bf/src/deploy/hash-files.js#L20
https://github.com/netlify/js-client/blob/4cb1f240bac527305ff98032a801accf22dea5bf/src/deploy/util.js#L12

This seems like the intended behavior in order to prevent walking all hidden directories (.git and others). It seems to me that the CLI should throw an error if the -d flag is passed a hidden directory.

@d4h0
Copy link
Author

d4h0 commented Sep 13, 2020

Throwing an error would already be an improvement, but why should it be impossible to deploy from a hidden directory? A hidden directory seems like a reasonable choice for generated files that the user normally doesn't interact with.

@monsonjeremy
Copy link
Contributor

Throwing an error would already be an improvement, but why should it be impossible to deploy from a hidden directory? A hidden directory seems like a reasonable choice for generated files that the user normally doesn't interact with.

I agree that it's a valid use case. I'm not very familiar with the code, but it seems it would require a bit more effort to build this feature.

I'd be happy to implement, just asking for some direction before taking this one.

@d4h0
Copy link
Author

d4h0 commented Sep 13, 2020

Okay, thanks. I guess it wouldn't be the end of the world if hidden directories are forbidden (just a little odd).

@monsonjeremy
Copy link
Contributor

@d4h0 I've opened two PR's to address the issue. If that isn't the desired outcome then it will be pretty easy to implement code to throw an error.

@erezrokah
Copy link
Contributor

Thanks @monsonjeremy, seems like this is also related to the second bullet here #1225 (comment)

@erezrokah
Copy link
Contributor

I was able to reproduce the issue, but only if there is no netlify.toml file in the project, see https://github.com/erezrokah/netlify-build-reproductions/tree/cli/issue_1227.

git clone git@github.com:erezrokah/netlify-build-reproductions.git
git checkout cli/issue_1227
netlify deploy -d .build

@d4h0 can you confirm that running touch netlify.toml (creates an empty file) serves as a workaround?

@monsonjeremy
Copy link
Contributor

monsonjeremy commented Sep 14, 2020

@erezrokah I can confirm the same behavior as you have seen. Interesting. Do you think we should still handle the case where there is no netlify.toml? If not, maybe we can throw an error if the directory is a hidden one and the netlify.toml file does not exist?

I'm not sure that adding netlify.toml actual causes all the files to be found? From my local testing, it logs out the following

✔ Finished hashing 1 files
...

I believe that is only the netlify.toml file that is being hashed though.

I tested this on the branch with my changes, and it logs out:

✔ Finished hashing 2 files
...

To me this confirms that the issue still exists, even with an empty netlify.toml.

@erezrokah
Copy link
Contributor

There are 2 separate issues here I think:

  1. Not having a netlify.toml makes the deploy stuck (happens when dir starts with a . or trying to deploy an empty directory).
  2. Netlify CLI ignore hidden files/directories while deploying.

@monsonjeremy
Copy link
Contributor

@erezrokah I agree, however I think they are both related. Fixing the second bullet will also fix the first one since the hidden directory will be picked up, allowing the deploy to work because files are found.

@erezrokah
Copy link
Contributor

Fixing the second bullet will also fix the first one since the hidden directory will be picked up, allowing the deploy to work because files are found.

Correct, but it will fix only one variation of the first bug. Trying to deploy an empty folder (without a netlify.toml file) will still get stuck).

@monsonjeremy
Copy link
Contributor

I can make a separate DX issue for throwing an error if trying to deploy an empty folder, it seems that is worth encompassing in a separate ticket with low priority. What do you think?

@erezrokah
Copy link
Contributor

Since this issue is about deployment getting hanged we can keep it as is.
We can open another issues about deploying hidden files/directories. I'll need to investigate the latter more as we have other reports of issues with the CLI deploy missing some files:
#1225 (comment)
I'll need to make sure the CLI deploy is synced with the way we do it in our build bot.

@monsonjeremy
Copy link
Contributor

Sounds good @erezrokah! Thanks for the clarification, really appreciate it. Do you think we should keep my two PR's open as is in the meantime, or should I open a new issue and reference those PR's in there?

@d4h0
Copy link
Author

d4h0 commented Sep 14, 2020

Since this issue is about deployment getting hanged we can keep it as is.
We can open another issues about deploying hidden files/directories.

Maybe I'm misunderstanding something, but I created this issue specifically because deploying a hidden directory failed (see the title of the issue ;))

@d4h0 can you confirm that running touch netlify.toml (creates an empty file) serves as a workaround?

I'm not sure that adding netlify.toml actually causes all the files to be found? From my local testing, it logs out the following

Yes, it seems only "netlify.toml" is processed. I have four files, and during a successful deployment (of a non-hidden directory), 5 files are processed (when "netlify.toml" exists). With a hidden directory (when "netlify.toml" exists), one file is processed, so I guess that's "netlify.toml".

I found out something new:

If I try to deploy a non-hidden folder that's in a hidden folder (".build/deploy"), everything works! :)

@erezrokah
Copy link
Contributor

Trying to clear the confusion - I believe the deploy gets stuck when the logic inside the js-client that looks for files to deploy tries to deploy an empty set of files.
That can happen when trying to deploy a hidden directory (due to the other issue of ignoring hidden files) or when trying to deploy an empty directory.
In both of the above cases adding a netlify.toml fixes the hang, but in the case of a hidden folder the files in the folder are not deployed.

I'm going to fix the empty set of files getting stuck issue first, and then take a deeper look at which files should be deployed. It seems that netlify build bot doesn't care about hidden files, so I think the CLI should not care too.

@d4h0
Copy link
Author

d4h0 commented Sep 15, 2020

Alright, thanks for the effort, @erezrokah.

@erezrokah
Copy link
Contributor

It seems that netlify build bot doesn't care about hidden files, so I think the CLI should not care too.

Following up on this specific case, buildbot does ignore hidden files:
https://github.com/netlify/open-api/blob/e72e2eb2d7eedf02bccc5916fc0330022f7f823b/go/porcelain/deploy.go#L692
but will still publish a hidden directory if used as a publish directory.
The CLI would ignore both (which is a bug).

@monsonjeremy
Copy link
Contributor

@erezrokah Does that mean we should change the CLI deploy command to allow hidden directories?

@erezrokah
Copy link
Contributor

@erezrokah Does that mean we should change the CLI deploy command to allow hidden directories?

Yes! Submitted a PR for this here #1272

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

Successfully merging a pull request may close this issue.

3 participants