-
Notifications
You must be signed in to change notification settings - Fork 71
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
decorrelate source and target directories #282
base: main
Are you sure you want to change the base?
decorrelate source and target directories #282
Conversation
Maintainers, As you review this RFC please queue up issues to be created using the following commands:
Issues(none) |
Signed-off-by: Xavier FRANCOIS <[email protected]>
85be549
to
e9fceb3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems reasonable to me. I'd be happy to steward this one. If implemented I could see this being a new input to the exporter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @xfrancois for contributing this!
I've given you some critiques in this review but I want to be clear that I'm not opposed to moving forward with this idea, just that I think it's better for all of us to explain the justification for doing so more clearly, and to consider whether it's possible to re-use the existing control points (env vars and flags for respective commands) at all or whether we'd be better off introducing two new ones that are mutually exclusive with the existing one.
# Summary | ||
[summary]: #summary | ||
|
||
Allow CNB users to build image from one directory and have the final image directory structure to be something else. Currently it's possible to change the default target directory (/workspace) by setting the `CNB_APP_DIR` environement variable, but the source directory must also be the one specified in `CNB_APP_DIR`. This behavior is not well suited for constrained environements, like CI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is almost a side-note, but the lifecycle exporter also takes a cmd line arg -app
that can override that same CNB_APP_DIR
. There's also a corresponding flag in pack, --path
or -p
for overriding the same. This might not be important from the spec perspective, but from an implementation perspective there's actually several things, that already have inconsistent naming, to separate and find new names for.
# Definitions | ||
[definitions]: #definitions | ||
|
||
N/A |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo it would be helpful here to define
"the place where the application source lives"
and
"the place where we will put the built artifacts after building the application"
Right now, both are referred to by the same name, but also both have 3 names (per the above list of flags and env vars). so i think it will be helpful for this document to find names that work, as those could help lead to unified flag and env var naming as well :)
|
||
- What use cases does it support? | ||
|
||
Building from a CI tool, for example Gitlab-CI. We can't chose the source directory from which we are building. On Gitlab-CI the Git folder is checkout on `/builds/<group>>/<project>`. By having a way to decorrelate source and target directories, we could chose the finaly directory structure we want. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first a nit: there's a couple typos here:
we could chose the finaly directory structure
should probably be:
"we could choose the final directory structure"
but more importantly, I still don't fully understand the why of this proposal. What do you gain by being able to choose your directory structure? What gets easier for you? What is blocked by a lack of this ability currently?
|
||
- What is the expected outcome? | ||
|
||
Support of a new environment variable / parameter that allows to set the source directory. We probably should document that `CNB_APP_DIR` only handles the target directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good, just see above as there seems to be 3 touch points not just the env var.
This provides a high level overview of the feature. | ||
|
||
- Define any new terminology. | ||
- Define the target persona: buildpack author, buildpack user, platform operator, platform implementor, and/or project contributor. | ||
- Explaining the feature largely in terms of examples. | ||
- If applicable, provide sample error messages, deprecation warnings, or migration guidance. | ||
- If applicable, describe the differences between teaching this to existing users and new users. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this part is just the boilerplate? should probably be deleted or filled in with your specific case :)
|
||
In the Gitlab-CI example, I can't chose the source directory but I want to be able to chose the target directory. I will set the variables like this : | ||
|
||
`CNB_SOURCE_APP_DIR`: /builds/group/project |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given the way the appPath is currently used in pack it may be more backwards compatible to keep the "APP_DIR" as the variable which means "both source and destination"
OR as the variable which means "source only" rather than as the variable that means "destination only".
Looking in lifecycle it also looks like it's pretty thoroughly "the source and destination" directory, so i wonder if it would be better to add 2 entirely new env vars and then do some validation that they're used exclusively with the existing env vars and flags?
|
||
# Drawbacks | ||
[drawbacks]: #drawbacks | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a little disingenuous to claim there's no drawbacks -- at the very least, any new variable is something that comes with a small maintenance and complexity cost.
|
||
- What is the impact of not doing this? | ||
|
||
The impact is that people wanting to use CNB on CI environment will have to make some workaround (like copying source folders in another location) in order to have a proper directory structure in the final image. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the impact of having an "improper" directory structure? What gets harder or worse other than an abstract sense of "proper-ness" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit complicated to explain but let me try :
On Gitlab-CI I'm building an application A that uses an app B as a "service" (equivalent to docker-compose in gitlab-ci).
This app B in my context has also been built with buildpack on Gitlab-CI. As it's been build on Gitlab-CI, by default the working directory is /workspaces, so we need to set up the CNB_APP_DIR to /worspaces. Then the final image of the application B would have a directory of /workspaces.
Back to the application A build : when launching the app B as a service, the directory structure of the B app will clash with Gitlab-CI default, and then the app B won't be able to run.
To sum up, the inability to chose the "proper" directory structure prevents us from running my app as a service dependency in Gitlab-CI context.
The only workaround that works is when building the "B" app, copy the content from /worskpaces (default Gitlab CI folder) to another location, set the CNB_APP_DIR to this other location. That way the final directory structure will not be "/workspaces" and it won't clash when loading this app as a service when building app A.
I have also documented this issue here : https://stackoverflow.com/a/75875777/3767820
Hope the need is clearer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @xfrancois - i think it's coming into focus but it's worth really understanding.
This situation arises when:
-
you are invoking the lifecycle directly, so you're managing your own "unit of hermetic building" via CI. (If you were using pack, the 'hermetic unit' would be the container created by pack, which is fresh for each invocation)
-
you want to build 2 (coupled) applications in the same "hermetic unit"; i.e. the build and run for apps A and B share the same filesystem.
-
you want two different images / or "runnable units"
-
you are constrained by your "provider of hermetic units" (your CI) to have a hardcoded directory name
-
and the problem is then that you wind up with two applications in a single directory?
-
and the current workaround is to manually copy a directory?
To be honest i'm still a little confused. Maybe you can draw a more detailed sketch. I'm not finding the word "proper" helpful as I think you're referring to a convention that I don't know, so it might help to spell out that convention?
Are you trying to wind up with one image that can run either app A or B?
Why wouldn't it work to do something like
cp -r path/to/appA/src /workspaceA
lifecycle ... -app /workspaceA
cp -r path/to/appB/src /workspaceB
lifecycle ... -app /workspaceB
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xfrancois any further thoughts here?
Proposition to decorrelate source and target directories
The idea is to introduce a new variable named
CNB_SOURCE_APP_DIR
, that could be specified to tell where the source code is. But the final image directory structure will still be created following theCNB_APP_DIR
variable.That way, source and target directories structure could be decorrelate.
This will fix this issue.