Skip to content

Generate config for list results #37173

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
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Generate config for list results #37173

wants to merge 2 commits into from

Conversation

dsa0x
Copy link
Member

@dsa0x dsa0x commented May 28, 2025

Fixes #

Target Release

1.13.x

Rollback Plan

  • If a change needs to be reverted, we will roll out an update to the code within 7 days.

Changes to Security Controls

Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.

CHANGELOG entry

  • This change is user-facing and I added a changelog entry.
  • This change is not user-facing.

@dsa0x dsa0x force-pushed the sams/list-genconfig branch from 0220cd9 to 5c3631d Compare May 28, 2025 11:26
@dsa0x dsa0x added the no-changelog-needed Add this to your PR if the change does not require a changelog entry label May 28, 2025
@dsa0x dsa0x force-pushed the sams/list-genconfig branch 2 times, most recently from 9c45eda to 31abfb0 Compare May 28, 2025 11:27
@dsa0x dsa0x force-pushed the sams/tfquery-execute branch from 24d0a55 to 439cb9e Compare May 28, 2025 15:28
@dsa0x dsa0x force-pushed the sams/list-genconfig branch from 49d6d13 to b1fec46 Compare June 2, 2025 20:36
@dsa0x dsa0x marked this pull request as ready for review June 2, 2025 20:37
@dsa0x dsa0x requested a review from a team as a code owner June 2, 2025 20:37
@dsa0x dsa0x force-pushed the sams/tfquery-execute branch 6 times, most recently from 0d9f62d to b0a7c2e Compare June 10, 2025 15:56
Base automatically changed from sams/tfquery-execute to sams/list-result-schema June 10, 2025 17:38
Base automatically changed from sams/list-result-schema to main June 10, 2025 18:08
@dsa0x dsa0x force-pushed the sams/list-genconfig branch from b1fec46 to 8c12ea0 Compare June 11, 2025 14:54
@dsa0x dsa0x requested review from jbardin and dbanck June 12, 2025 12:22
@dsa0x dsa0x force-pushed the sams/list-genconfig branch from a4fd5e9 to 68a752e Compare June 16, 2025 17:34
jbardin
jbardin reviewed Jun 23, 2025
View reviewed changes
internal/terraform/node_resource_plan_instance_query.go Outdated
},
DeposedKey: states.NotDeposed,
}

ctx.Changes().AppendResourceInstanceChange(change)
return diags
}

func (n *NodePlannableResourceInstance) generateListConfig(obj cty.Value, resourceSchema providers.Schema) (generated string, diags tfdiags.Diagnostics) {
Copy link
Member

Choose a reason for hiding this comment

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

This is almost entirely a copy of generateHCLStringAttributes, is there any way we can combine them?

jbardin
jbardin reviewed Jun 23, 2025
View reviewed changes
Copy link
Member

@jbardin jbardin left a comment

Choose a reason for hiding this comment

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

This looks like a good start, I'm sure we're going to be coming back to this code as we figure out how code generation interacts with the cloud UI. It would be good if we can find a way reduce any copying between the import generation and this though, so we can evolve them together.

@dsa0x dsa0x force-pushed the sams/list-genconfig branch from 68a752e to 4b2f4cb Compare June 24, 2025 14:41
@dsa0x dsa0x changed the base branch from main to dbanck/tfquery-cli June 24, 2025 14:54
@dsa0x dsa0x force-pushed the sams/list-genconfig branch from 4b2f4cb to d77f275 Compare June 24, 2025 14:59
@dsa0x dsa0x force-pushed the sams/list-genconfig branch from d77f275 to bbe3024 Compare June 24, 2025 16:21
@dbanck dbanck force-pushed the dbanck/tfquery-cli branch 3 times, most recently from 912b10f to 679b585 Compare June 30, 2025 09:56
@dsa0x dsa0x force-pushed the sams/list-genconfig branch 2 times, most recently from e331e73 to 052b03c Compare July 1, 2025 06:21
@dbanck dbanck force-pushed the dbanck/tfquery-cli branch from 679b585 to 2066ea6 Compare July 1, 2025 07:58
@dsa0x dsa0x force-pushed the sams/list-genconfig branch from 16dce47 to 8050ca1 Compare July 1, 2025 09:13
@dbanck dbanck force-pushed the dbanck/tfquery-cli branch from a270992 to e5bb256 Compare July 2, 2025 08:28
Base automatically changed from dbanck/tfquery-cli to main July 2, 2025 13:06
@dsa0x dsa0x requested a review from a team as a code owner July 2, 2025 13:10
@dsa0x dsa0x force-pushed the sams/list-genconfig branch from 1ed8a7c to fe3c957 Compare July 2, 2025 13:28
dbanck
dbanck requested changes Jul 2, 2025
View reviewed changes
Copy link
Member

@dbanck dbanck left a comment

Choose a reason for hiding this comment

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

Nice work, but I encountered some issues while testing this.

I also wasn't able to have the config written to a file (when not using -json). Maybe we need to update the backends to write the config

internal/terraform/node_resource_plan_instance_query.go
@@ -100,6 +100,16 @@ func (n *NodePlannableResourceInstance) listResourceExecute(ctx EvalContext) (di
return diags
}

// If a path is specified, generate the config for the resource
if n.generateConfigPath != "" {
Copy link
Member

Choose a reason for hiding this comment

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

This needs to happen before the hook is called. Otherwise results.Generated will always be null

internal/command/views/hook_json.go

result := json.NewQueryResult(addr, value)
Copy link
Member

Choose a reason for hiding this comment

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

We only want to include config in the JSON when it has been requested and generated. Otherwise we will run into a panic

internal/command/views/json/query.go Outdated
Address: addr.String(),
func NewQueryResult(listAddr addrs.AbsResourceInstance, value cty.Value, generated *genconfig.Resource) QueryResult {
result := QueryResult{
Address: generated.Addr.String(),
Copy link
Member

Choose a reason for hiding this comment

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

The address was a reference to the list block that produced this result. We should introduce another field if we want to expose the generated reference as well. But do we need that?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Perhaps we do not. I will leave it for now

internal/command/views/json/query.go Outdated
ResourceObject: marshalValues(value.GetAttr("state")),
// TODO: Add config once we have it available
Config: string(generated.Body),
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be generated.String() to output the full resource block with labels

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbardin jbardin jbardin left review comments

@dbanck dbanck dbanck requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
no-changelog-needed Add this to your PR if the change does not require a changelog entry
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dsa0x @jbardin @dbanck