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

Add Lifecycle Configuration File #128

Closed
wants to merge 4 commits into from

Conversation

jabrown85
Copy link
Contributor

@jabrown85 jabrown85 commented Dec 17, 2020

@jabrown85 jabrown85 requested a review from a team as a code owner December 17, 2020 18:02
In order to allow platforms to have more dynamic configurations, a new configuration file (`lifecycle.toml`) could be loaded by lifecycle binaries.

Signed-off-by: Jesse Brown <jabrown85@gmail.com>
Signed-off-by: Jesse Brown <jabrown85@gmail.com>

An example `lifecycle.toml` might be produced by the platform with various inputs (source code, platform specific configuration, etc.):
```
[config]
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should try to keep these keys matching the lifecycle flags exactly. So:

  • app instead of app_dir
  • order instead of order_path

OTOH, these more closely match the env vars

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went ahead and changed the names to be kebab-case, but I kept them matching the env variants for now. I was thinking it might make sense to be able to set some things that maybe aren't available as flags in the future. Like CNB_REGISTRY_AUTH isn't a flag, but you still might want to do the work to get that auth setup in the same pod. Similarly, there may be things like lifecycle feature flags or version selection that could be useful to have here that is likely to be available as an env var but may not have a flag name.

Signed-off-by: Jesse Brown <jabrown85@gmail.com>
Signed-off-by: Jesse Brown <jabrown85@gmail.com>
[alternatives]: #alternatives

- What other designs have been considered?
- Lifecycle could introduce a `prepare` phase that processes a project's `project.toml` to generate the input files needed by detector, much like `pack` does today.
Copy link
Member

Choose a reason for hiding this comment

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

does the implementation of prepare change anything for you here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If prepare outputs an order.toml and puts it in a location that detector picks up, I would be OK putting a pause on this RFC. That is my primary need right now.

@nebhale nebhale requested a review from a team January 13, 2021 20:33
- What other designs have been considered?
- Lifecycle could introduce a `prepare` phase that processes a project's `project.toml` to generate the input files needed by detector, much like `pack` does today.
- Lifecycle could load env vars from a known file location. e.g. `source lifecycle.env` or similar prior to resolving lifecycle configuration.
- Lifecycle could only allow for non-path based inputs like `log-level` in `config.toml`, but add an additional path to to a known platform location. Example would be `/cnb/config/order.toml` for `CNB_ORDER_PATH`. This would allow platforms to use a volume at `/cnb/config` and write files that are fed into lifecycle in addition to a `config.toml` for non-path based configuration.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Lifecycle could only allow for non-path based inputs like `log-level` in `config.toml`, but add an additional path to to a known platform location. Example would be `/cnb/config/order.toml` for `CNB_ORDER_PATH`. This would allow platforms to use a volume at `/cnb/config` and write files that are fed into lifecycle in addition to a `config.toml` for non-path based configuration.
- Lifecycle could only allow for non-path based inputs like `log-level` in `config.toml`, but add an additional path to a known platform location. Example would be `/cnb/config/order.toml` for `CNB_ORDER_PATH`. This would allow platforms to use a volume at `/cnb/config` and write files that are fed into lifecycle in addition to a `config.toml` for non-path based configuration.

# How it Works
[how-it-works]: #how-it-works

Lifecycle will read in `CNB_CONFIG_PATH` or the default of `/cnb/config.toml` and prioritize the values found over lifecycle's defaults and before present env vars.
Copy link
Contributor

@yaelharel yaelharel Jan 21, 2021

Choose a reason for hiding this comment

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

What is the prioritization of the cli flag vs a configuration file?
I guess it is:

  1. cli flags
  2. env vars
  3. configuration file
  4. default

right? Maybe it's worth adding it.

** edit **
Now I see that it appears at the end of the "What it is" section.
Anyway, I think that it's worth adding the prioritization of the cli flags to this sentence.

log-level = "debug"
```

An example config resolution would be, in prioritized order, CLI flag, os env `CNB_ORDER_PATH`, `[config.toml][config]order_path`, then the `lifecycle` default of `/cnb/order.toml`.
Copy link
Contributor

Choose a reason for hiding this comment

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

What about env vars that are also defined in the config file?
For example:
log-level = "debug" in the config file and EnvLogLevel as a variable that the user manually set.

@jabrown85
Copy link
Contributor Author

I'm going to close this for now. This spec change will get me most of the way there by being able to optionally provide an order.toml that does not belong to the builder. Thanks for the review comments, I'll make sure to bring them in if I resurrect this.

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.

6 participants