Skip to content

Conversation

@luschmar
Copy link
Contributor

Add functionality Variables to a Manifest in ApplicationManifestUtils

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 25, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@luschmar luschmar requested a review from dmikusa May 31, 2022 06:47
@dmikusa
Copy link
Contributor

dmikusa commented May 31, 2022

Thanks for adding the additional tests.

I do think this should error/throw an exception though if an env variable is not substituted. It would be consistent with the cf cli behavior, which is I think what we should strive for with the operations library (it's higher level).

> cf push
Expected to find variables: foo
FAILED

It may not be specifically listed as invalid to have an app name including ((foo)) but the flip side is that if you have an env variable set and accidentally forget to set it to something that could have ramifications as well. Maybe you push the app with the wrong name or wrong route or wrong service bound. It's safer to throw an error/exception and let the user handle that.

You as a user could simply catch that exception and ignore it, if you don't care, but you can't do the opposite.

@luschmar luschmar requested a review from dmikusa June 1, 2022 09:44
@dmikusa dmikusa merged commit f834b53 into cloudfoundry:main Jun 1, 2022
@dmikusa
Copy link
Contributor

dmikusa commented Jun 1, 2022

Thanks @luschmar !

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

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants