[config] look for the devbox.json file in parent directories as well #200
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
savil
force-pushed
the
savil/subfolder-search-config
branch
from
c74e5ad to
207074a
Compare
savil
force-pushed
the
savil/subfolder-search-config
branch
from
207074a to
16b811f
Compare
gcurtis
reviewed
Oct 4, 2022
devbox.go
Outdated
Comment on lines
247
to
253
| if cur == "" || cur == "." { | ||
| var err error | ||
| cur, err = os.Getwd() | ||
| if err != nil { | ||
| return "", errors.WithStack(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
I think you need to call filepath.Abs to handle all the relative path cases. For example, if the config is in ./repo and the user runs devbox .. from ./repo/a.
There was a problem hiding this comment.
The filepath.Abs was being called inside pathArgs: https://github.com/jetpack-io/devbox/blob/7ae5af019d7f568bb50e164401fa6b293a22e7c5/boxcli/args.go#L18
which is then passed in here.
But I do like the suggestion to sanitize the dir here as well via calling filepath.Abs
There was a problem hiding this comment.
Yeah, it might be a good idea to sanitize it here too. Otherwise it's not super obvious that this method requires it.
gcurtis
approved these changes
Oct 4, 2022
savil
force-pushed
the
savil/subfolder-search-config
branch
2 times, most recently
from
626eeb1 to
a4901dc
Compare
1 task
savil
force-pushed
the
savil/subfolder-search-config
branch
from
a4901dc to
8b8e7ab
Compare
|
will re-submit in the hopes that helps with the auto-checks running properly. |
savil
added a commit
that referenced
this pull request
Oct 6, 2022
…, and not combined with srcDir (#212) ## Summary ~This should fix a consequence of a change introduced in #200 to make the devbox.srcDir be an absolute path. In the BuildPlanners, we need the paths to be relative so they are inside the "docker context" and can be used in Dockerfiles.~ New Fix: I think the problem is actually scoped to just the NodeJSPlanner. Within NodeJSPlanner, the inputFiles were being set using `filepath.Join(srcDir, <filename>)` when instead it should be just `<filename>`, or more pedantically `path/of/filename/from/devbox-json`. This is because the docker-context is the directory-of-devbox-json, and any files `COPY --link`'d should be specified with paths relative to this docker-context. I inspected `git grep InputFiles` and it seems NodeJSPlanner is the only one with this issue. I also went back in history to a commit from Monday, built the binary and found that the following would fail due to this reason: ``` > cd testdata/nodejs > devbox build nodejs-18 ``` It works now with this fix. ## How was it tested? in www.jetpack.io repo, 1. did `devbox build` and `docker run -p 3000:3000 --expose 3000 -ti devbox` and could open the website in localhost:3000 2. did `cd root-of-www.jetpack.io` and did `devbox build www && docker run -p 3000:3000 --expose 3000 -ti devbox`. Works! 3. did `mkdir -p root-of-www.jetpack.io/www/fake-dir && cd root-of-www.jetpack.io/www/fake-dir` and then: `devbox build ../ && docker run -p 3000:3000 --expose 3000 -ti devbox`. Works! Also, works with just `devbox build` i.e. omitting `../` argument (due to #200) For NodeJS testdata in `testdata/nodejs`: 1. `cd testdata/nodejs` and `devbox build nodejs-18 && docker run devbox`. Works! 2. `cd testdata/nodejs/nodejs-18` and `devbox build && docker run devbox`. Works! 3. `cd testdata/nodejs/nodejs-18/fake-dir` and `devbox build && docker run devbox`. Works! in devbox repo, did `devbox build` and `docker run devbox` with: - `testdata/rust/rust-stable` - `testdata/python/pip-example`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Motivation
Previously, a user would need to specify the directory of the devbox.json when
invoking
devbox shellordevbox buildand similar commands. In the absence,of an explicit directory argument, we would assume the user is saying that
the devbox.json file is in the current directory.
This PR changes this to look for the devbox.json file in the specified-directory, or current directory if none specified,
AND any parent directory.
This is desirable for a few reasons:
and be able to invoke
devbox shellordevbox build, in a manner similar tohow
gitworks.devbox addanddevbox rmto run from any sub-directory. These commandscurrently require the user to be located in the "directory of the devbox.json".
With this PR, this requirement is relaxed so that the commands can be run more naturally
in any sub-directory as well.
Fixes #185
How was it tested?
devbox shellanddevbox buildinside a sub-directorydevbox shellanddevbox buildin a directory containing a devbox.jsondid these commands in
testdata/rust/rust-stableexplicitly specify the folder of the devbox.json