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

Allow main package to be at non-root path of app #2

Merged
merged 7 commits into from
Jun 19, 2019

Conversation

SemanticallyNull
Copy link

It was not possible to build an application if it had a main package that was not in the root of the workspace. This PR creates an environment variable which will allow users to set a path.

Introduces BP_APP_MAIN_PATH environmental variable

  • defaults to "." if BP_APP_MAIN_PATH not set

With @iainsproat

@cfdreddbot
Copy link

✅ Hey BenChapman! The commit authors and yourself have already signed the CLA.

@sclevine
Copy link
Member

sclevine commented May 3, 2019

Sorry for the delay! We're still working on Tracker integration for the new buildpacks.

I think we'd like this to be configurable via a buildpack.yml file instead of an environment variable. Maybe something like this:

gomod:
  paths: ["some/cmd/here", "another/cmd/overhere"] 

@scothis
Copy link

scothis commented May 3, 2019

It's common to have multiple targets within a single repo that should each be compiled into separate images. The target package should be configurable per build.

@sclevine
Copy link
Member

sclevine commented May 3, 2019

That makes sense. I think we should also support multiple targets in the same image as well (e.g., for extra binaries for DB migrations, debug tools, etc.). The v2 version of the buildpack support multiple targets via GO_INSTALL_PACKAGE_SPEC.

We're trying to capture all of the build configuration in buildpack.yml and only introduce environment variables where parameters could reasonably vary across builds. I agree that this is one of those cases -- I should have considered that in my previous comment.

How about we support a default set of targets in buildpack.yml:

go-mod:
  targets: ["some/cmd/here", "another/cmd/overhere"] 

As well as an environment variable to override that set:

BP_GO_MOD_TARGETS=some/cmd/different:another/cmd/overhere

@scothis
Copy link

scothis commented May 3, 2019

@sclevine sounds good.

If there are multiple targets, how do you set the entry point?

@sclevine
Copy link
Member

sclevine commented May 3, 2019

CMD is set to run the binary built from the first target.

iainsproat and others added 3 commits June 7, 2019 12:37
* env var BP_GO_MOD_TARGETs
* allows multiple packages to be specified, delimited by colon :
Targets to build in the app directory can be defined in yaml file `buildpack.yml` at the
root of the application file

Added integration tests
[#166378921]

Signed-off-by: Shane Huston <shuston@pivotal.io>
[#165933655]

Signed-off-by: Katie Chapman <kchapman@pivotal.io>
@SemanticallyNull
Copy link
Author

SemanticallyNull commented Jun 7, 2019

@sclevine the update that we just pushed should fix the functionality to what you described in a previous comment.

/cc @hustons @iainsproat @cmccarthy101

[#165933655]

Signed-off-by: Shane Huston <shuston@pivotal.io>
mod/mod.go Outdated Show resolved Hide resolved
mod/mod_test.go Outdated Show resolved Hide resolved
@sclevine
Copy link
Member

sclevine commented Jun 7, 2019

This is looking great!

We discussed the format of buildpack.yml today and decided that language-specific tooling should be nested under a language key. Happy for whoever merges to adjust this, but it should be either:

go:
  mod:
    targets: ["./path/to/my"]

or:

go:
  targets: ["./path/to/my"]

depending on if the targets concept is general enough to apply to all ways of building go apps (e.g., dep). I think it is, so I support the second option.

@SemanticallyNull
Copy link
Author

SemanticallyNull commented Jun 11, 2019

We've made those changes, we used the latter as the way to do things.

We're currently investigating an issue where the environment variable doesn't actually work, despite the tests passing. Once we've figured it out we'll update the PR and add a test to make sure this doesn't happen again.

Please don't merge yet

This was because we were trying to pass the environment variable into the pack CLI, but it needs to be passed as a flag. The environment variable works as expected when passed into pack using the -e flag.

@zmackie zmackie merged commit e0d3de3 into paketo-buildpacks:master Jun 19, 2019
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.

7 participants