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

fix(core): fix default environment resolution #2051

Merged
merged 2 commits into from
Sep 14, 2020

Conversation

thsig
Copy link
Collaborator

@thsig thsig commented Sep 14, 2020

What this PR does / why we need it:

This fixes a regression in the resolution flow for template strings in project.defaultEnvironment.

@thsig thsig force-pushed the fix-default-environment-resolution branch from 9a7b6ce to 79a2cb9 Compare September 14, 2020 10:17
core/test/unit/src/config/project.ts Outdated Show resolved Hide resolved
@@ -1,5 +1,6 @@
kind: Project
name: test-project-templated
defaultEnvironment: "${local.env.TEST_VARIABLE == 'banana' ? 'local' : 'local'}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where and how is this actually tested?

Copy link
Collaborator Author

@thsig thsig Sep 14, 2020

Choose a reason for hiding this comment

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

This test project is used in the "should resolve templated env variables in project config" case in the unit tests for the Garden class.

Before the fix, it failed with the error reported by Mitchell (and now no longer fails).

It's essentially just verifying that template strings are, in fact, resolved on config.defaultEnvironment in Garden.factory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would definitely have been cleaner to do this inline in the test case, but Garden.factory reads the file from the FS.

Maybe I should add a comment to this config referring to the test case where it's used?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nah it's alright for this instance, but yeah going forward we should inline the configuration where possible.

This fixes a regression in the resolution flow for template strings in
`project.defaultEnvironment`.
@thsig thsig force-pushed the fix-default-environment-resolution branch from 79a2cb9 to d523407 Compare September 14, 2020 10:37
@thsig thsig merged commit 19e5f55 into master Sep 14, 2020
@thsig thsig deleted the fix-default-environment-resolution branch September 14, 2020 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants