Waypoint application input vars #833
Conversation
* Add variable type to application input vars * Set variable options on a template to be computed * Enforce uniqueness in a template's variable options, if provided * Add acceptance test for templates where a template is user editable, instead of using variable options
This commit updates Waypoint application creation to omit the `waypoint_application` input variable from the variables returned to the provider from HCP Waypoint. This variable is implicitly set by the service, and is never expected to be in the config. Since it's not in the config, but is returned by the API, if it is added to the state, Terraform errors since the config and state don't match, even though that is expected. Another change in this PR is that the type of the input variable for an application is set in the state based on what is in the config, NOT the API response. This is because the API presently does not return the type of the variable. If and when that changes, this can be undone (as noted in the comment with this change).
Basic test case is updated to only test basic aspects of a template, not variables with options. That has been split into a new test case.
* Implement value converter interface for input vars, to use "As" for conversion from a TF value to a struct * Refactor code to read input vars into the plan/state into readInputs function * Set list of input vars to a null list if there are no inputs * Update tests to use corrected variable options * Add variable type to data source schema, though it won't be set yet
This test simulates a template which sets two variables and an application created using that template.
The list of input vars removed by this commit is only needed during creation of an app, not a read.
This commit splits up the input vars for an application that are set by a template and an app into two separate parts of the schema for the resource. The function readInputs is updated to make the distinction.
paladin-devops
force-pushed
the
WAYP-2173-app-input-vars
branch
from
aab535a
to
3120752
Compare
Update acc test to test that template vars are set for an application that uses a template which configured a value for an input var.
The variable type for the template-configured input variables of an application is optional. This will remain true unless the API changes to always return a variable's type.
The waypoint_application data source was updated to no longer use the `readInputs` method created to help with reading input vars for the waypoint_application resource. Instead it's handled in a loop in the data source's read function, because it doesn't split vars between app and tpl input vars. Tests were also updated accordingly here.
| // One might see an opportunity here to use an embedded struct to avoid code duplication; | ||
| // however, this is not currently possible in the framework. See this issue for more details: | ||
| // https://github.com/hashicorp/terraform-plugin-framework/issues/242 | ||
| TemplateInputVars types.Set `tfsdk:"template_input_vars"` |
There was a problem hiding this comment.
Depending upon how this issue is addressed, we may be able to simplify this struct to embed ApplicationDataSourceModel into this one.
| } | ||
|
|
||
| // FromTerraform5Value implements the ValueConverter interface | ||
| func (vc *varConverter) FromTerraform5Value(val tftypes.Value) error { |
There was a problem hiding this comment.
| func (vc *varConverter) FromTerraform5Value(val tftypes.Value) error { | |
| func (vc *varConverter) FromTerraformValue(val tftypes.Value) error { |
Is this a typo?
There was a problem hiding this comment.
FromTerraform5Value is correct, it implements the ValueConverter interface, allowing us to use As here.
There was a problem hiding this comment.
FYI @HenryEstberg I ultimately ended up removing this code after chatting with @catsby and pivoting to using ElementsAs to do the conversion, over implementing the conversion in the loop later in this file via As (which is what used this interface implementation).
| @@ -80,6 +80,26 @@ func (d *DataSourceApplication) Schema(ctx context.Context, req datasource.Schem | |||
| Computed: true, | |||
| Description: "Internal Namespace ID.", | |||
| }, | |||
| "app_input_vars": schema.SetNestedAttribute{ | |||
There was a problem hiding this comment.
Should we name them simply input_variables? The app prefix seems to stutter a bit (application.app_input..) and in the Template resource we use the full word variables and not just vars
internal/provider/waypoint/data_source_waypoint_application_template_test.go
Show resolved
Hide resolved
| return | ||
| } | ||
|
|
||
| resp.Diagnostics.Append(readInputs(ctx, inputVars, data, varTypes)...) |
There was a problem hiding this comment.
there is indirection here that I think is unnecessary. Is there a reason to embed the readInputs function call inside this Append? What about readInputs makes it particularly suited to return diagnostics as opposed to simply formatting and returning two []*InputVar and we do the SetValueFrom inline with this READ method, and do resp.Diagnostics.AddError there?
There was a problem hiding this comment.
Returning diagnostics (which is returned by the call to types.SetValueFrom within readInputs) helps reduce code duplication, because if we returned two []*InputVar we'd need to make the SetValueFrom calls wherever readInputs is called. It's not a large amount of duplicated code though, so I'm OK with moving the SetValueFrom to the calling function side to reduce the unnecessary indirection.
Co-authored-by: Clint <[email protected]>
The set of input vars for the Waypoint application data source should be null if there are not any variables. Co-authored-by: Clint <[email protected]>
Doing this is necessary for drift detection. Co-authored-by: Clint <[email protected]>
Co-authored-by: Clint <[email protected]>
This is simpler than implementing the value converter interface.
Typo in new field name "input_variables" was fixed. The # of variables also has changed since `waypoint_application` is counted among the variables.
The # of variables has changed since `waypoint_application` is counted among the variables.
paladin-devops
force-pushed
the
WAYP-2173-app-input-vars
branch
from
0cfd6a7
to
470d078
Compare
🛠️ Description
Add support for input vars to Waypoint applications.
On the
waypoint_applicationresource, there is a distinction for how an input variable was set.app_input_varsare configurable by thewaypoint_applicationresource.template_input_varsare input variables that are set for an application, but were configured by the template, which was used for the creation of the app. Thewaypoint_applicationdata source does not make this distinction, and all input variables are "filed under" theinput_varsattribute of the data source.Until the HCP Waypoint API is updated to return this information (if it ever does), the
waypoint_applicationresource assumes that the type of an input variable in the TF configuration for the resource is the type, so long as the name of the variable matches. Arguably, this assumption should be made by the API. If/when this changes, the code to work around this should be removed, and the provider should defer to the type that the API says an input variable is.Acceptance test output:
Fixes WAYP-2173.