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

[RFC 0081] Add process-specific working directories to io.buildpacks.build.metadata label #216

Closed
Tracked by #1422
ekcasey opened this issue Mar 31, 2021 · 5 comments · Fixed by #290
Closed
Tracked by #1422

Comments

@ekcasey
Copy link
Member

ekcasey commented Mar 31, 2021

RFC 0081
https://github.com/buildpacks/rfcs/pull/144/files

To allow platforms to inspect all information related to process types, we should add process-specific working directories to to processes in io.buildpacks.build.metadata:

{
  "processes": [
    {
      "type": "<process-type>",
      "command": "<command>",
      "args": [
        "<args>"
      ],
      "direct": false,
      "working-directory": "<working-directory>"
    }
 ...
}

Open Questions:
If the buildpack did not set a working directory for the process should this key be omitted or set to the app directory (which will be the working directory)? I am inclined to say it should be omitted b/c the platform could theoretically change the app dir at runtime. This admittedly is an edge case but I could imagine someone attempting a development workflow where a user mounts a modified app into the container. pack could be more opinionated and populate the app dir in the inspect-image output if it is omitted from the label.

@ekcasey ekcasey added this to the Platform 0.7 milestone Mar 31, 2021
@samj1912
Copy link
Member

samj1912 commented Apr 5, 2021

Should this be merged with #212 ?

@natalieparellano
Copy link
Member

I am inclined to say it should be omitted

I agree with you @ekcasey !

@natalieparellano
Copy link
Member

natalieparellano commented Jan 27, 2022

Question on the implementation:

If a newer buildpack (that supports working directory) is run on an older platform, what should be the behavior?

  • Should the provided working directory be respected at launch (since the launch behavior and /layers/config/metadata.toml are specified in the platform api). Current implementation: it is respected. If it were NOT respected, users would need to upgrade their platform to get the newer buildpack behavior, which doesn't make sense.
  • Should the provided working directory be included in the label? My reading of the spec is that it should NOT be included, because platforms could be parsing the label and error on an unexpected key. We would need to "sanitize" the label for the newer platform api...

This is another feature where the buildpack api and the platform api are joined together. I wonder if there's any way to disentangle them... getting rid of /layers/config/metadata.toml would probably help.

@ekcasey
Copy link
Member Author

ekcasey commented Feb 2, 2022

@natalieparellano

This is another feature where the buildpack api and the platform api are joined together. I wonder if there's any way to disentangle them... getting rid of /layers/config/metadata.toml would probably help.

I agree that getting rid of metadata.toml would go a long way towards helping us keep the APIs independent in general. In this particular case we would still have the problem b/c of the label. Theoretically putting the process types in a label is unnecessary because it serves no strictly-necessary functional purpose, but it is very useful for platform like pack that want to provide diagnostic functional like pack inspect.

I think we should:

  1. Always respect the working dir regardless of platform API
  2. Always set the label regardless of platform API

Because:

  1. This is a buildpack features, the platform API parts are merely informational
  2. Changes to anything a buildpacks might interact with (the label) are purely additive

If we wanted to be sticklers about this I would take the whole process schema out of the spec and put something generic like "buildpack-provided process definition goes here" but I don't think that is in anyone's best interest currently.

@natalieparellano
Copy link
Member

natalieparellano commented Feb 16, 2022

I believe #290 and the current lifecycle implementation reflects the intended state of the world...

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

Successfully merging a pull request may close this issue.

4 participants