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: Combine and organize metadata file locations across lifecycle #145

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

samj1912
Copy link
Member

@samj1912 samj1912 commented Mar 11, 2021

Readable

It's my first time creating an rfc so not sure if I did everything right here.

Signed-off-by: Sambhav Kothari <skothari44@bloomberg.net>
@samj1912 samj1912 requested a review from a team as a code owner March 11, 2021 13:57
@samj1912 samj1912 changed the title Disambiguate layer metadata files from app metadata RFC: Disambiguate layer metadata files from app metadata Mar 11, 2021
…proach

Signed-off-by: Sambhav Kothari <skothari44@bloomberg.net>
@samj1912
Copy link
Member Author

I updated the rfc with the sub-directory proposal instead of the prefix one as discussed in the last WG meeting.

@sclevine sclevine requested a review from a team March 31, 2021 19:11
@jromero
Copy link
Member

jromero commented Apr 5, 2021

I updated the rfc with the sub-directory proposal instead of the prefix one as discussed in the last WG meeting.

This makes it sound like the sub-directory structure would be the primary idea but I'm still seeing prefixes as the primary proposal. Am I missing something?

@samj1912
Copy link
Member Author

samj1912 commented Apr 5, 2021

I updated the rfc with the sub-directory proposal instead of the prefix one as discussed in the last WG meeting.

This makes it sound like the sub-directory structure would be the primary idea but I'm still seeing prefixes as the primary proposal. Am I missing something?

Urgh - I think I forgot to push. Should be fixed now.

@jromero
Copy link
Member

jromero commented Apr 6, 2021

Really like the directory restructure. The term buildpack-workspace is a bit of a mouthful so I may bikeshed on it a bit but it totally makes sense IMO.

Signed-off-by: Sambhav Kothari <skothari44@bloomberg.net>
Copy link
Contributor

@jabrown85 jabrown85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the structure change.

@natalieparellano
Copy link
Member

Really like the directory restructure. The term buildpack-workspace is a bit of a mouthful so I may bikeshed on it a bit but it totally makes sense IMO.

buildspace?

@ekcasey
Copy link
Member

ekcasey commented Apr 14, 2021

Capturing a discussion from WG, Stephen proposed:

  1. Moving "top-level" files to a different "config" directory so that the layers dir provided to the buildpack is exclusively for layers and their metadata.
  2. Providing the location of both the layers and config directories as environment variables, rather than positional arguments

Elaborating on this idea and digging the details we would need to work out:

current directory structure:

`/layers` (configurable with `CNB_LAYERS_DIR` or -layers) 
 ├── group.toml
 ├── plan.toml
 └── <escaped_buildpack_id> (buildpack arg `$1` )
    ├── build.toml
    ├── launch.toml
    ├── store.toml
    ├── <layer>.toml
    └── <layer>
  └── config
    └── metadata.toml

Assuming we want to reusing the existing layers volume for the new files instead of introducing a new required volume, I think these are the possible future directory structures?
Option 1:

`/layers` (configurable with `CNB_LAYERS_DIR` or -layers) 
 ├── group.toml
 ├── plan.toml
 └── <escaped_buildpack_id> (`CNB_BP_LAYERS_DIR` )
    ├── <layer>.toml
    └── <layer>
  └── config
     └── metadata.toml
     └── <escaped_buildpack_id> (`CNB_BP_CONFIG_DIR` )
          ├── build.toml
          ├── launch.toml
          └── store.toml

pros:

  • doesn't break layer reuse
  • same-length for layer path names
    cons:
  • calling the top-level layers even though it is layers + config

Option 2:

`/build` (configurable with `CNB_BUILD_DIR` or -build-dir) 
 ├── group.toml
 ├── plan.toml
 └── l ( or `layers?`)
      └── <escaped_buildpack_id> (`CNB_BP_LAYERS_DIR` )
          ├── <layer>.toml
          └── <layer>
 └── c ( or `config`?)
      ├── metadata.toml
      └── <escaped_buildpack_id> (`CNB_BP_CONFIG_DIR`  )
          ├── build.toml
          ├── launch.toml
          └── store.toml

pros:

  • doesn't incorrectly call the entire top level directory layers (it's really too bad we used workspace for the app dir).
    cons:
  • breaks layer reuse
  • layers/layers? layers/l? (both are bad, one is shorter)
  • longer path names
  • right now we export layers/config, if we implement this before eliminating metadata.toml we would need to partially export /layers/config.

In the above examples names of specific directories or env vars could change. E.g:

  1. Do we want to reuse CNB_LAYERS_DIR to mean something different in the buildpack context instead of a creating a new CNB_BP_LAYERS_DIR?
  2. build is just a placeholder. workspace actually seems the most logical to me but we already used it for the app dir.

But I think the overall structures represent the two obvious options.

Other possibilities that I won't take as much time to elaborate on:

  1. add a layers and config dir under each buildpack ID
  2. unite layers and the app dir in a single mega workspace (e.g. workspace/app and workspace/<bp_id> or workspace/config, and workspace/layers) -- this one would be nice but it would also require a lot of changes...

@jkutner
Copy link
Member

jkutner commented Apr 15, 2021

I'm +1 on @ekcasey's "option 1". I'm also fine with /layers/config. I don't think it's exceptionally weird.

Copy link
Member

@sclevine sclevine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bikeshed

│ └── plan.toml
│ └── report.toml
├── layers (configurable with `CNB_LAYERS_DIR` or -layers)
│ └── new-buildpack (`CNB_BP_LAYERS_DIR`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since CNB_BP_LAYERS_DIR is used more often than CNB_LAYERS_DIR, could we pick a shorter name for the former?

Maybe something like:

  • CNB_ALL_LAYERS_DIR for /workspace/layers
  • CNB_ALL_CONFIG_DIR for /workspace/layers-config
  • CNB_LAYERS_DIR for /workspace/layers/<buildpack-id>
  • CNB_CONFIG_DIR for /workspace/layers-config/<buildpack-id>?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a minor concern with using CNB_LAYERS_DIR for CNB_BP_LAYERS_DIR since the former is already a config var for /layers. We would be moving CNB_LAYERS_DIR from a platform env var to a buildpack env var, which feels a bit weird.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point. Maybe CNB_ALL_LAYER_DIR and CNB_LAYER_DIR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still like CNB_BP_LAYERS_DIR for its obviousness.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now the buildpack's layers dir is provided to it as a positional arg

/bin/build <layers[EIC]> <platform[AR]> <plan[ER]>, Working Dir: <app[AI]>

Are we proposing replacing all positional args in the buildpack API with environment variables as part of that RFC? J I think that is a good idea but it's a big enough change that I think we should call this out explicitly in a separate section and provide the names for the env vars that will replace the plan and platform arguments as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go with CNB_BP_LAYERS_DIR should we switch from CNB_BUILDPACK_DIR to CNB_BP_DIR for consistency?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to change them to _BUILDPACK_ to match CNB_BUILDPACK_DIR. They are long, but explicit and match our current standards that buildpacks are already built against.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ekcasey I would prefer to not tackle the positional argument changes in this RFC.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we aren't switching from position arguments to environment variables then I am unsure what CNB_BUILDPACK_LAYERS_DIR is referring to above. The layers dir is currently a positional argument and nobody would be setting or reading this environment variable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if we are introducing at least one environment variable CNB_BUILDPACK_CONFIG_DIR it does seem to me like an appropriate time to convert the rest (just for the sake of internal consistency).

text/0000-layer-metadata-files.md Outdated Show resolved Hide resolved
├── plan.toml
├── order.toml (written by a platform, optionally)
├── report.toml
├── <escaped_buildpack_id> (buildpack arg `$1`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For new buildpacks running on the old platform, do we set CNB_BP_CONFIG_DIR equal to CNB_BP_LAYERS_DIR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we would need to.

text/0000-layer-metadata-files.md Outdated Show resolved Hide resolved
text/0000-layer-metadata-files.md Outdated Show resolved Hide resolved
text/0000-layer-metadata-files.md Outdated Show resolved Hide resolved
text/0000-layer-metadata-files.md Outdated Show resolved Hide resolved
text/0000-layer-metadata-files.md Show resolved Hide resolved
text/0000-layer-metadata-files.md Outdated Show resolved Hide resolved
│ └── plan.toml
│ └── report.toml
├── layers (configurable with `CNB_LAYERS_DIR` or -layers)
│ └── new-buildpack (`CNB_BP_LAYERS_DIR`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go with CNB_BP_LAYERS_DIR should we switch from CNB_BUILDPACK_DIR to CNB_BP_DIR for consistency?

Changes based on feedback

Signed-off-by: Jesse Brown <jabrown85@gmail.com>
@ekcasey ekcasey self-requested a review August 25, 2021 15:28
Signed-off-by: Jesse Brown <jabrown85@gmail.com>
Comment on lines +92 to +99
│ └── lifecycle (configurable with `CNB_LIFECYCLE_CONFIG_DIR`)
│ ├── analyzed.toml
│ ├── group.toml
│ ├── plan.toml
│ └── report.toml
│ └── platform (configurable with `CNB_PLATFORM_CONFIG_DIR`)
│ ├── order.toml (written by a platform, optionally - prior to `lifecycle` execution)
└ └── project-metadata.toml (written by a platform, optionally - prior to `lifecycle` execution, configurable with `CNB_PROJECT_METADATA_PATH`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this make sense to go in the platform directory? In talking through buildpacks/pack#1221 it seems like it could be helpful for the platform to mount a directory where it could provide configuration and receive output.

│ └── <layer>
├── config
│ └── layers (configurable with `CNB_LAYERS_CONFIG_DIR`)
│ ├── metadata.toml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a child of config?

@hone
Copy link
Member

hone commented Oct 6, 2021

Putting on the backburner for now due to other active RFCs.

@buildpack-bot
Copy link
Member

Maintainers,

As you review this RFC please queue up issues to be created using the following commands:

/queue-issue <repo> "<title>" [labels]...
/unqueue-issue <uid>

Issues

(none)

@hone
Copy link
Member

hone commented Oct 6, 2021

@natalieparellano suggested of pairing this with deprecation of other APIs.

│ └── <layer>
└── config
└── metadata.toml
/workspace (configurable with `CNB_APP_DIR ` or -app)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to change the default location for the app from /workspace to /app. Could we incorporate that change into this RFC too?

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

Successfully merging this pull request may close these issues.

None yet