Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
chore: Uses NewUnknownReplacements meant to replace
schemafunc.CopyUnknowns
logic andschemafunc.NewAttributeChanges
#3192New 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
base: master
Are you sure you want to change the base?
chore: Uses NewUnknownReplacements meant to replace
schemafunc.CopyUnknowns
logic andschemafunc.NewAttributeChanges
#3192Changes from 31 commits
b04314c
ed85e27
9ffd580
10d8a82
19a9f90
a2bac7b
cbd65fb
7e4004b
3e8b7d0
3cc4e05
77eb77b
dda909b
b728367
d9c59d4
ce1e5c5
9d3f769
cbf31d9
4d70f84
2233dc5
ec64b97
9684396
938c6b4
7e22345
8d9093b
d710412
3bbec61
9e37725
c8ce3cd
c1ac25b
19d305b
7229da8
0fe2780
8d87c23
a714bdb
47aa17f
df15b83
13825e5
bc6aeb1
02cae8f
e958361
10ebaf4
d7deeb3
40a66a9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the internal function:
func AttributePath(ctx context.Context, tfType *tftypes.AttributePath, schema fwschema.Schema) (path.Path, diag.Diagnostics) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't gone deep, but it smells a lot we're copying internal methods. Also, what is the impact on the policy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we get any additional information here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Waiting on reply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider having a helper func for the 3 errors in the func
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you tell more about IsSetIndex and why is different from the rest, here we do Contains instead of HasSuffix. Can you give an example of set index to see how it looks like and how the suffix is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example:
"advanced_configuration.custom_openssl_cipher_config_tls12[Value(\"ECDHE-RSA-AES256-GCM-SHA384\")]"
Want to separate IsListIndex from IsMapIndex and IsSetIndex.
See new docstrings in
plan_modify_differ.go#L140
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, so I don't still understand why we want to differentiate IsSetIndex and IsMapIndex, can we just have IsMapIndex? what is a use case where we want to treat them differently?
(also note IsMapIndex is not correct bc you have to do something like in IsListIndex impl., say if IsSetIndex(p) return false)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a subtle difference:
"replication_specs[Value(\"myKey\")]"
ends with)]
, close parenthesis+square bracket"replication_specs[\"myKey\"]"
ends with"]
quote+square bracketSee also test case
TestIndexMethods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I missed that different end, thanks. and when do we want to treat them differently?
I was considering index when it's a number like [number], else set or map index
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far
set
andmap
are used with primitive valuesstring
in this case. While the ListIndex is used with nested objects. We don't need to check forSetLenChanges
orMapLenChanges
onlyListLenChanges
so far