Check the size of `build_path`

nkaretnikov commented 10 months ago

Fixes #649.

netlify[bot] commented 10 months ago

Deploy Preview for kaleidoscopic-dango-0cf31d canceled.

Name	Link
Latest commit	e2a7618c14249f86c186812214c710f1f543a5a7
Latest deploy log	https://app.netlify.com/sites/kaleidoscopic-dango-0cf31d/deploys/65511ef73490960008ffc55d

nkaretnikov commented 10 months ago

TODO:

[x] One of the CI integration tests fails. I think this might be this check, due to CI having a longer conda-store state dir location.
[x] I think this needs a unit test, to make sure the error is raised.
[x] A simple integration test would be trying to create an environment with a very long environment name. This affects the build_key and hence the build_path/prefix, so should error out.
- I also want to check the effect of this on the UI. Will it throw 500 with an error printed to the console? Can we show a friendlier error message in the UI?
- Related bug: https://github.com/conda-incubator/conda-store/issues/654
- By finding the number of 'a's that would not exceed the filename limit, it's possible to hit the error I'm adding in this PR, but as with the above bug, it's currently not shown in the UI
- Should be fixed. Unfortunately, there's no way to communicate an error message right now because it's only appended to the logs, since schema.BuildStatus.FAILED doesn't contain an error message. I'll see if this can be added easily.
[x] See if it's possible to report more info with status FAILED.

nkaretnikov commented 10 months ago

This PR should be merged first since it has new tests, which are useful. I think it will conflict with #652 because it uses a migration (migration hashes won't match). #531 might require rebasing as well.

nkaretnikov commented 10 months ago

Companion PR on the UI side (see screenshot there): https://github.com/conda-incubator/conda-store-ui/pull/332

nkaretnikov commented 9 months ago

I think we can afford to be more detailed here, but I don't know if that's the correct place. Maybe just codifying the error type with an identifier+number we can add some documentation on how to work around this issue?

I initially printed the whole path and its size, too. But that'd be flaky on CI because different directories are used everywhere. It also seems like an implementation detail, which is not helpful unless you have admin rights. I'll just add documentation for now and we can improve later.

nkaretnikov commented 9 months ago

Hey @jaimergp, thanks for your comments! I'm going to document the build_path in docs/administration.md, similar to how we document the MAX_PATH issue on Windows. Otherwise, I don't think there's a need to change anything else.

UPD: Added documentation.

trallard commented 9 months ago

@jaimergp is this ready for merge? if so then perhaps you and/or @nkaretnikov should open a follow-up issue with potential improvements or areas to explore and merge this straight away

conda-incubator / conda-store