Merged
Conversation
denik
requested review from
andrewnester,
anton-107,
pietern and
shreyas-goenka
as code owners
denik
changed the title
Collaborator
16 failing tests:
|
denik
force-pushed
the
denik/structaccess-type
branch
from
dfc22cd to
948cf8b
Compare
libs/structaccess/typecheck.go
Outdated
| // Fields marked readonly/internal are considered inaccessible. | ||
| btag := structtag.BundleTag(sf.Tag.Get("bundle")) | ||
| if btag.Internal() || btag.ReadOnly() { | ||
| return fmt.Errorf("%s: field %q not found in %s", newPrefix, c.Key(), owner.String()) |
Contributor
There was a problem hiding this comment.
Shall we be more specific here and say that we're trying to access internal field?
Contributor
Author
There was a problem hiding this comment.
It's a good idea. However, this code is actually unused -- bundle tags filtered out earlier, so I remove this here: 7131d9c
The reason earlier code filters it out and not doing hard error is because we might still have success later with non-internal field.
libs/structaccess/typecheck.go
Outdated
| // Fields marked readonly/internal are considered inaccessible. | ||
| btag := structtag.BundleTag(sf.Tag.Get("bundle")) | ||
| if btag.Internal() || btag.ReadOnly() { | ||
| return fmt.Errorf("%s: field %q not found in %s", newPrefix, c.Key(), owner.String()) |
Contributor
There was a problem hiding this comment.
Also, why can't we use ReadOnly?
Contributor
Author
There was a problem hiding this comment.
Currently it is only used for "id" which is handled separately.
| @@ -0,0 +1,8 @@ | |||
| resources: | |||
Contributor
There was a problem hiding this comment.
It looks like TF fails to deploy with all of ingestion_definition,ingestion_definition.0.objects must be specified but direct is not, why is that? and is it intended? TF error looks valid though
Contributor
Author
There was a problem hiding this comment.
This is a validation that TF does. In direct we don't do any validations (but important ones we should add to initialize phase). I added objects so that it passes TF validation -- 2274f77
andrewnester
approved these changes
Aug 28, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Changes
Why
I'm going to add lookups in remote state. Remote state is optional / needs a request to backend. Type checking the path allows checking if reference is valid for remote state without actually having the state.
Tests
New acceptance test to show value-based non-existent field.
More non-regression test that cover different types of access and show inconsistencies between terraform and direct.