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.

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

views: add cloud view #35929

Conversation

bschaatsbergen
Copy link
Member

@bschaatsbergen bschaatsbergen commented Oct 30, 2024

Closes #35159

The cloud backend implements a retry mechanism for its client, go-tfe. Currently, retry messages from this logic were not rendered through a view, so the -json flag was not respected. This resulted in mixed (non-pure JSON) output in the terminal whenever retry messages were printed.

This PR introduces a new view: cloud, which handles rendering output in human-readable or machine-readable format (JSON) from cloud-specific operations.

The PR works around the current lack of native support for views in the backends. To work around this limitation, we expose the ViewType and View to the CLI options, which we pass down to the cloud backend.

 $ go test -run "TestNewCloud" -v
=== RUN   TestNewCloudHuman_RetryLog
=== RUN   TestNewCloudHuman_RetryLog/first_retry,_no_output
=== RUN   TestNewCloudHuman_RetryLog/second_retry,_server_error
=== RUN   TestNewCloudHuman_RetryLog/subsequent_retry_with_elapsed_time
=== RUN   TestNewCloudHuman_RetryLog/retry_with_429_status,_no_output
--- PASS: TestNewCloudHuman_RetryLog (0.00s)
    --- PASS: TestNewCloudHuman_RetryLog/first_retry,_no_output (0.00s)
    --- PASS: TestNewCloudHuman_RetryLog/second_retry,_server_error (0.00s)
    --- PASS: TestNewCloudHuman_RetryLog/subsequent_retry_with_elapsed_time (0.00s)
    --- PASS: TestNewCloudHuman_RetryLog/retry_with_429_status,_no_output (0.00s)
=== RUN   TestNewCloud_unsupportedViewDiagnostics
--- PASS: TestNewCloud_unsupportedViewDiagnostics (0.00s)
=== RUN   TestNewCloud_humanViewOutput
=== RUN   TestNewCloud_humanViewOutput/no_param
=== RUN   TestNewCloud_humanViewOutput/single_param
--- PASS: TestNewCloud_humanViewOutput (0.00s)
    --- PASS: TestNewCloud_humanViewOutput/no_param (0.00s)
    --- PASS: TestNewCloud_humanViewOutput/single_param (0.00s)
=== RUN   TestNewCloud_humanViewDiagnostics
--- PASS: TestNewCloud_humanViewDiagnostics (0.00s)
=== RUN   TestNewCloudJSON_RetryLog
=== RUN   TestNewCloudJSON_RetryLog/attempt_0,_no_output
=== RUN   TestNewCloudJSON_RetryLog/attempt_1,_server_error
=== RUN   TestNewCloudJSON_RetryLog/subsequent_retry_with_elapsed_time
=== RUN   TestNewCloudJSON_RetryLog/retry_with_429_status,_no_output
--- PASS: TestNewCloudJSON_RetryLog (0.00s)
    --- PASS: TestNewCloudJSON_RetryLog/attempt_0,_no_output (0.00s)
    --- PASS: TestNewCloudJSON_RetryLog/attempt_1,_server_error (0.00s)
    --- PASS: TestNewCloudJSON_RetryLog/subsequent_retry_with_elapsed_time (0.00s)
    --- PASS: TestNewCloudJSON_RetryLog/retry_with_429_status,_no_output (0.00s)
=== RUN   TestNewCloud_jsonViewOutput
=== RUN   TestNewCloud_jsonViewOutput/no_param
=== RUN   TestNewCloud_jsonViewOutput/single_param
--- PASS: TestNewCloud_jsonViewOutput (0.00s)
    --- PASS: TestNewCloud_jsonViewOutput/no_param (0.00s)
    --- PASS: TestNewCloud_jsonViewOutput/single_param (0.00s)
=== RUN   TestNewCloud_jsonViewDiagnostics
--- PASS: TestNewCloud_jsonViewDiagnostics (0.00s)
PASS
ok      github.com/hashicorp/terraform/internal/command/views   0.902s

The cloud backend includes a retry hook that relies on a go-tfe implementation. Since these retry messages were not rendered through a view, the `-json` flag was not respected, leading to mixed (non-pure JSON) output in the terminal whenever retry messages were printed.

Before passing the init view to the cloud backend, we ensure the retryLog hook is included in the init view.

Signed-off-by: Bruno Schaatsbergen <git@bschaatsbergen.com>
Signed-off-by: Bruno Schaatsbergen <git@bschaatsbergen.com>
Signed-off-by: Bruno Schaatsbergen <git@bschaatsbergen.com>
@bschaatsbergen bschaatsbergen force-pushed the b/fix-retry-messages-in-cloud-backend-json branch from f9d9df0 to f979759 Compare November 1, 2024 10:44
@bschaatsbergen bschaatsbergen force-pushed the b/fix-retry-messages-in-cloud-backend-json branch from ecbb274 to cb2d019 Compare November 1, 2024 11:38
Signed-off-by: Bruno Schaatsbergen <git@bschaatsbergen.com>
@bschaatsbergen bschaatsbergen force-pushed the b/fix-retry-messages-in-cloud-backend-json branch from cb2d019 to a909304 Compare November 1, 2024 11:45
The cloud view is responsible for rendering messages related to cloud operations, presenting them to the user in either a human-readable format or as JSON output in the terminal.

Signed-off-by: Bruno Schaatsbergen <git@bschaatsbergen.com>
…based on view type.

Signed-off-by: Bruno Schaatsbergen <git@bschaatsbergen.com>
Signed-off-by: Bruno Schaatsbergen <git@bschaatsbergen.com>
Signed-off-by: Bruno Schaatsbergen <git@bschaatsbergen.com>
Added a comment to explain the rationale behind the guard statement, which prevents potential nil errors during backend initialization and retryLogHook call.

Signed-off-by: Bruno Schaatsbergen <git@bschaatsbergen.com>
Signed-off-by: Bruno Schaatsbergen <git@bschaatsbergen.com>
Signed-off-by: Bruno Schaatsbergen <git@bschaatsbergen.com>
@bschaatsbergen bschaatsbergen changed the title views: move retryLog hook to init view views: add cloud view Nov 5, 2024
@bschaatsbergen bschaatsbergen force-pushed the b/fix-retry-messages-in-cloud-backend-json branch from 932541a to c71a1a7 Compare November 6, 2024 12:52
Signed-off-by: Bruno Schaatsbergen <git@bschaatsbergen.com>
Signed-off-by: Bruno Schaatsbergen <git@bschaatsbergen.com>
@bschaatsbergen bschaatsbergen force-pushed the b/fix-retry-messages-in-cloud-backend-json branch from fc57ccc to 94c732b Compare November 6, 2024 20:00
@bschaatsbergen bschaatsbergen marked this pull request as ready for review November 8, 2024 20:05
@bschaatsbergen bschaatsbergen requested a review from a team as a code owner November 8, 2024 20:05
Copy link
Member

@liamcervante liamcervante left a comment

Choose a reason for hiding this comment

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

I think you need to run make copyright-fix to pass the code consistency checks!

internal/command/views/cloud.go Outdated Show resolved Hide resolved
t.RetryLogHook(attemptNum, resp, false),
"type", json.MessageTestRetry,
)
t.Cloud.RetryLog(attemptNum, resp)
Copy link
Member

Choose a reason for hiding this comment

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

You could get rid of the TFCRetryHook function entirely now, and just call the RetryLog function directly from wherever this was being called before.

Signed-off-by: Bruno Schaatsbergen <git@bschaatsbergen.com>
Copy link
Contributor

@sebasslash sebasslash left a comment

Choose a reason for hiding this comment

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

These changes look great 😄. I suppose an open question, of which I don't have a strong opinion, is whether we want to suppress these messages unless a log level is set by TF_LOG. I personally prefer verbose output, but it may introduce more noise than some people would prefer.

Some optional 💅 suggestions below

internal/command/views/cloud.go Outdated Show resolved Hide resolved
internal/command/views/cloud.go Show resolved Hide resolved
internal/command/views/cloud.go Outdated Show resolved Hide resolved
Copy link
Member

@liamcervante liamcervante left a comment

Choose a reason for hiding this comment

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

Looks good to me! Probably good to wait for Sebastian's approval too

Copy link
Contributor

@sebasslash sebasslash 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!

@bschaatsbergen bschaatsbergen merged commit f8cb25a into hashicorp:main Nov 13, 2024
5 of 6 checks passed
Copy link
Contributor

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@bschaatsbergen bschaatsbergen deleted the b/fix-retry-messages-in-cloud-backend-json branch November 13, 2024 21:35
bschaatsbergen added a commit to bschaatsbergen/terraform that referenced this pull request Nov 14, 2024
…retry-messages-in-cloud-backend-json"

This reverts commit f8cb25a, reversing
changes made to a25831d.
dbanck pushed a commit to dbanck/terraform that referenced this pull request Nov 15, 2024
…retry-messages-in-cloud-backend-json"

This reverts commit f8cb25a, reversing
changes made to a25831d.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Print retryLogHook messages to stderr
3 participants