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.

Sign up for GitHub

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

Jump to bottom

tfsdk: Prevent Attribute misconfiguration by consolidating Computed, Optional, and Required fields into a single Configuration field #95

Closed
wants to merge 2 commits into from
Closed

tfsdk: Prevent Attribute misconfiguration by consolidating Computed, Optional, and Required fields into a single Configuration field #95

wants to merge 2 commits into from

Conversation

bflad
Copy link
Member

@bflad bflad commented Aug 10, 2021

Closes #31
Closes #94

Field, type, and value naming (and where the bits live) is up for discussion. 😄

@bflad
tfsdk: Prevent Attribute misconfiguration by consolidating Computed, …
01c1fbf 
…Optional, and Required fields into a single Configuration field

Reference: #31
Reference: #94
@bflad bflad added the enhancement New feature or request label Aug 10, 2021
@bflad bflad requested a review from a team August 10, 2021 18:01
paddycarver
paddycarver reviewed Aug 16, 2021
View reviewed changes
Copy link
Contributor

@paddycarver paddycarver left a comment

Choose a reason for hiding this comment

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

The code seems fine and straightforward enough to me.

I'm not sold on the naming or the positioning of the abstraction, I think. Configuration already means something, and is pretty similar to Config which is used in requests already and I'm worried about overloading terms here.

Typing out these long names seems pretty tedious. :/

And I'm still 🤔 if we should name the values for their impacts or for how they're represented in schema. :/

tfsdk/attribute.go
// value must be configured by the practitioner. Must be defined.
//
// See AttributeConfiguration descriptions for additional information.
Configuration AttributeConfiguration
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm anticipating a lot of confusion over this naming.

@bflad
Copy link
Member Author

bflad commented Aug 18, 2021

Given the semantic differences between where the value is expected to be sourced from (configuration vs the provider) and plan difference handling (optional vs optional and computed) the abstraction/naming here is going to be confusing likely no matter what we do. Going to punt on this for now, since its not critical path, in preference of just addressing #94 in a followup PR using the existing functionality.

@bflad bflad closed this Aug 18, 2021
@bflad bflad deleted the f-Attribute-Configuration-field branch August 18, 2021 20:18
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@paddycarver paddycarver paddycarver left review comments

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
2 participants
@bflad @paddycarver