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 ability for a resource to "use" another resources output automatically #72

Merged
merged 14 commits into from
Jan 13, 2022

Conversation

heyealex
Copy link
Contributor

@heyealex heyealex commented Jan 6, 2022

Implements the "use" field as described in the design document. This field allows a resource to depend on the outputs of another in the same resource group, which means that similar to global variables, any matching outputs names for the used variable will be paired up with a setting in the using resource, assuming none have been explicitly set yet.

There are a lot of opportunities with this in place to shorten our current examples. All available ones have been added already, but I plan to follow-up with another PR that makes output name/setting name changes to make the most of this new feature.

Submission Checklist:

  • Have you installed and run this change against pre-commit? pre-commit install
  • Are all tests passing? make tests
  • If applicable, have you written additional unit tests to cover this
    change?
  • Is unit test coverage still above 80%?
  • Have you updated any application documentation such as READMEs and user
    guides?
  • Have you followed the guidelines in our Contributing document?

Ensure that resource IDs passed into the use field exist and are in the
same resource group as the current resource.
Changes CheckResourcesAndGroupNames to be a standalone function that
returns error and ResourceToGroup map.

Also removed the -c flag from run_examples to remove the deprecation
warning.
Added a step in exand that applies any matching output in "use"
resources that matches the input of the current resource. In addition,
tests have been reorganized to decrease the size of the test_config.go
file.
Add tests for:
* getResourceByID
* useResource
* appendAsSlice
* getResourceByVarName

Various fixes to support tests also made
Adding a few function comments and cleaning up a test config
@heyealex heyealex requested a review from cboneti January 6, 2022 23:35
This commit adds the ability to use multiple resource output lists as a
single setting by merging the lists. This is done using the flatten
function in terraform and with a new feild in BlueprintConfig with
functions to be applied to settings in the main.tf file.
Copy link
Contributor Author

@heyealex heyealex left a comment

Choose a reason for hiding this comment

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

@cboneti It should be ready to review again. The applyFunctions implementation will probably have to change, but I wanted you to take a look before I acted on it.

.pre-commit-config.yaml Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
Copy link
Member

@cboneti cboneti left a comment

Choose a reason for hiding this comment

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

Please see my comments.
Please make sure we are testing the full precedence order for: default values, global vars, use, and then explicit settings.

cmd/create.go Outdated Show resolved Hide resolved
pkg/reswriter/tfwriter.go Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/expand.go Outdated Show resolved Hide resolved
pkg/config/expand.go Show resolved Hide resolved
pkg/config/expand.go Outdated Show resolved Hide resolved
pkg/config/expand.go Outdated Show resolved Hide resolved
pkg/config/expand.go Show resolved Hide resolved
For simplicity, move applyFunctions into the YamlConfig so it can be
directly accessed during and after expand in the same way. This allows a
call to expand to produce a YAML that can be used again in create with
no differences in the final result.

The name was also changed to WrapSettingsWith to be more explanatory and
fit better under the Resource struct.
Moves expand() and validate() to the expand.go and validate.go file
respectively for clarity and to keep config.go as top-level as possible.
Makes a number of changes in the useResource and applyUseResource
function to decrease complexity and increase readabiltiy.
Copy link
Member

@cboneti cboneti left a comment

Choose a reason for hiding this comment

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

Questions:

  1. did you test all the flow as we discussed? explicitly defined, then use, then global, then default?
  2. did you test expanding?
  3. did you test creating a config based on the expanded?

Thanks!

pkg/config/expand.go Outdated Show resolved Hide resolved
pkg/config/expand.go Outdated Show resolved Hide resolved
pkg/config/expand.go Show resolved Hide resolved
pkg/config/expand.go Show resolved Hide resolved
pkg/config/expand.go Show resolved Hide resolved
if wrap, ok := res.WrapSettingsWith[setting]; ok {
if len(wrap) != 2 {
return fmt.Errorf(
"invalid length of WrapSettingsWith, expected 2 got %d", len(wrap))
Copy link
Member

Choose a reason for hiding this comment

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

I would also say for which resource and which setting, if these are readily available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

* Pass in resource Inputs and Used resource outputs directly to
UseResource rather than the ResourceInfo structs
* Improve Comment Clarity
* Improve error output details
@heyealex
Copy link
Contributor Author

All feedback has been addressed, just waiting on the checks now.

@heyealex heyealex merged commit 40d20ff into GoogleCloudPlatform:develop Jan 13, 2022
@heyealex heyealex deleted the use-resources branch January 13, 2022 23:18
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