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

Serial port datasource #3247

Merged
merged 6 commits into from
Mar 17, 2020
Merged

Conversation

slevenick
Copy link
Contributor

Fixes: hashicorp/terraform-provider-google#1578

Fixes: hashicorp/terraform-provider-google#1472

The ability to read serial port output enables windows password generation.

Release Note Template for Downstream PRs (will be copied)

`google_compute_instance_serial_port`

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 252 insertions(+))
Terraform Beta: Diff ( 4 files changed, 252 insertions(+))

@slevenick slevenick requested a review from chrisst March 12, 2020 18:39
Copy link
Contributor

@chrisst chrisst left a comment

Choose a reason for hiding this comment

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

My biggest concern here is the unbounded nature of this polling loop. It doesn't timeout or fail if the API continues to respond with a non empty response. Is it possible to calculate a max number of queries that will be required before which it should fail? Or can this have a configurable read timeout added? Since this will be queried on every refresh it makes me a bit nervous.

contents := output.Contents
// When we reach the end of the serial port output stream the contents comes back empty
for output.Contents != "" {
output, err = config.clientCompute.Instances.GetSerialPortOutput(project, zone, d.Get("instance").(string)).Port(port).Start(output.Next).Do()
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth adding a debug log line here. It could be helpful understanding if this happens infinitely or takes longer than expected.

Copy link
Contributor

@chrisst chrisst left a comment

Choose a reason for hiding this comment

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

My biggest concern here is the unbounded nature of this polling loop. It doesn't timeout or fail if the API continues to respond with a non empty response. Is it possible to calculate a max number of queries that will be required before which it should fail? Or can this have a configurable read timeout added? Since this will be queried on every refresh it makes me a bit nervous.

Copy link
Contributor

@chrisst chrisst left a comment

Choose a reason for hiding this comment

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

My biggest concern here is the unbounded nature of this polling loop. It doesn't timeout or fail if the API continues to respond with a non empty response. Is it possible to calculate a max number of queries that will be required before which it should fail? Or can this have a configurable read timeout added? Since this will be queried on every refresh it makes me a bit nervous.

@slevenick
Copy link
Contributor Author

My biggest concern here is the unbounded nature of this polling loop. It doesn't timeout or fail if the API continues to respond with a non empty response. Is it possible to calculate a max number of queries that will be required before which it should fail? Or can this have a configurable read timeout added? Since this will be queried on every refresh it makes me a bit nervous.

Good point.

I removed the polling loop because I don't think its feasible to have a serial port output that won't fit in one request (output is limited to the last 1MB by the API).

If we run into issues in the future where someone wants this we can add back, but I don't expect that.

@slevenick slevenick requested a review from chrisst March 16, 2020 17:29
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 243 insertions(+))
Terraform Beta: Diff ( 4 files changed, 243 insertions(+))

@slevenick slevenick merged commit 99b90e2 into GoogleCloudPlatform:master Mar 17, 2020
nathkn pushed a commit to nathkn/magic-modules that referenced this pull request May 18, 2020
* Add serial port datasource

* Add doc for serial port ds

* Serial port test

* Formatting

* Markdown fixes

* Remove pagination, we should always be able to retrieve 1MB
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Serial Port data source No support for Automated Windows Password generation
4 participants