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

pkg/asset/cluster: Don't read empty-string state files #954

Conversation

wking
Copy link
Member

@wking wking commented Dec 20, 2018

Avoid the confusing:

ERROR Failed to read tfstate: open : no such file or directory

and backing syscall when terraform.Apply returns an empty string.

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 20, 2018
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 20, 2018
pkg/asset/cluster/cluster.go Outdated Show resolved Hide resolved
@wking wking force-pushed the ignore-tfstate-load-on-empty-path branch 2 times, most recently from bffc929 to 68e550f Compare February 4, 2019 22:38
Avoid the confusing:

  ERROR Failed to read tfstate: open : no such file or directory

and backing syscall when terraform.Apply returns an empty string.

We're not exiting immediately on Apply errors (and are instead
attempting to recover the Terraform state regardless of Apply errors)
since 78c3118 (pkg: add ClusterMetadata asset,type that can be used
for destroy, 2018-09-25, openshift#324).  The goal there is to retain the
current resource state in case the user wants to investigate the
resources associated with the partially-launched cluster and
potentially invoke Terraform themselves to finish the install instead
of deleting and re-creating their cluster.

Apply can return an empty-string state file since 3f4fe57
(cmd/openshift-install: Add 'destroy bootstrap' command, 2018-10-18,
leave the temporary directory without a terraform state file.  And we
don't care about recovering the state file in those cases, because we
haven't created any external resources at that point (before the apply
call).

Also flatten some unnecessary nesting in the err2 handling by using an
'else if' clause.
@wking wking force-pushed the ignore-tfstate-load-on-empty-path branch from 68e550f to 9d30577 Compare February 4, 2019 22:54
@wking
Copy link
Member Author

wking commented Feb 5, 2019

All the tests that matter are green :).

@staebler
Copy link
Contributor

staebler commented Feb 5, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 5, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: staebler, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@wking
Copy link
Member Author

wking commented Feb 6, 2019

Looks like e2e-aws 3446 succeeded, so...

/refresh

@wking
Copy link
Member Author

wking commented Feb 6, 2019

Ah, well. Giving up on job 3446...

/test e2e-aws

@wking
Copy link
Member Author

wking commented Feb 7, 2019

/test e2e-aws

@openshift-merge-robot openshift-merge-robot merged commit 39807ba into openshift:master Feb 7, 2019
@wking wking deleted the ignore-tfstate-load-on-empty-path branch February 7, 2019 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants