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 Default App Directory #128

Closed
wants to merge 1 commit into from
Closed

Fix Default App Directory #128

wants to merge 1 commit into from

Conversation

hone
Copy link
Member

@hone hone commented Mar 29, 2019

per lifecycle#77 the default app directory shouldn't be prefixed with /app

@hone hone requested review from jkutner, sclevine and ekcasey March 29, 2019 23:53
@hone hone force-pushed the fix-default-app-directory branch from e2a8576 to 2db74f2 Compare March 29, 2019 23:54
@jkutner
Copy link
Member

jkutner commented Mar 30, 2019

i don't know why, but when i pull this branch locally, i see:

$ git diff master..fix-default-app-directory
...
@@ -419,8 +403,7 @@ func (b *BuildConfig) build(ctx context.Context, lifecycle *build.Lifecycle) err
                "builder",
                build.WithArgs(
                        "-buildpacks", buildpacksDir,
-                       "-layers", layersDir,
-                       "-app", appDir,
+                       "-layers", launchDir,
                        "-group", groupPath,
                        "-plan", planPath,
                        "-platform", platformDir,

unsure why Github doesn't show that for me

@jkutner
Copy link
Member

jkutner commented Mar 30, 2019

i think it just needs a rebase

@hone hone force-pushed the fix-default-app-directory branch from 2db74f2 to 89218c4 Compare March 30, 2019 02:29
@sclevine
Copy link
Member

@ekcasey I see some other places that reference /workspace/app or look like they might need to be updated for the directory rename: https://github.com/buildpack/pack/blob/master/build.go#L77

@hone
Copy link
Member Author

hone commented Mar 30, 2019

my bad, the rebase fixed it locally for me. sorry :( going to close this.

@hone hone closed this Mar 30, 2019
@ekcasey
Copy link
Member

ekcasey commented Mar 30, 2019

@sclevine probably shouldn't have put buildpacks/lifecycle#77 in done. The lifecycle changes are done which was higher priority b/c we wanted to finish lifecycle beta first so that folks could create their beta builders.

More work need to be done to allow pack to use the new defaults. We need to create a second volume for the app. Until that is done pack need to use the flags to explicitly set values to the original default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants