Skip to content

Add some warnings around virsh-cleanup#87

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
cgwalters:virsh-cleanup-warning
Aug 1, 2018
Merged

Add some warnings around virsh-cleanup#87
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
cgwalters:virsh-cleanup-warning

Conversation

@cgwalters
Copy link
Member

I actually had some other VMs; luckily in my case they're all
easy to recreate, but others may not be in the same situation.

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 31, 2018
@abhinavdahiya
Copy link
Contributor

/assign @wking @yifan-gu

Copy link
Member

Choose a reason for hiding this comment

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

[[ and =~ are Bash-isms. Can we use:

echo "Press control-C to abort, or enter to continue."
read

Copy link
Member Author

Choose a reason for hiding this comment

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

Can I just change it to /bin/bash instead?

Copy link
Member

Choose a reason for hiding this comment

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

Can I just change it to /bin/bash instead?

The Control-C/enter approach seems usable enough to me that I don't think it's worth breaking POSIX compat. But most folks will have Bash, so I don't feel too strongly about it. And I'm not an approver anyway ;).

Copy link
Member

@wking wking Jul 31, 2018

Choose a reason for hiding this comment

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

An alternative to require an explicit Y/y in POSIX would be something like

printf 'Warning: This will destroy effectively all libvirt resources\nContinue [yN]? '
read CONTINUE
if test "${CONTINUE}" != y -a "${CONTINUE}" != Y
then
	echo 'Aborted' >&2
	exit 1
fi

Copy link
Contributor

Choose a reason for hiding this comment

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

@cgwalters I don't want to break POSIX compatibility since I depend on that to use these scripts. If it has to be bash, you need to use #!/usr/bin/env bash, but I like @wking's first suggestion better.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, done with @wking 's suggestion ⬇️

I actually had some other VMs; luckily in my case they're all
easy to recreate, but others may not be in the same situation.
@cgwalters cgwalters force-pushed the virsh-cleanup-warning branch from 1bd7d07 to ffc01c6 Compare August 1, 2018 00:02
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 1, 2018
@wking
Copy link
Member

wking commented Aug 1, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2018
@crawford
Copy link
Contributor

crawford commented Aug 1, 2018

/lgtm

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, crawford, wking

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

The pull request process is described here

Details 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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 1, 2018
@wking
Copy link
Member

wking commented Aug 1, 2018

The e2e-aws error was:

Waiting for router to be created ...
NAME                           STATUS    ROLES     AGE       VERSION
ip-10-0-141-50.ec2.internal    Ready     etcd      3m        v1.11.0+d4cacc0
ip-10-0-158-123.ec2.internal   Ready     etcd      3m        v1.11.0+d4cacc0
ip-10-0-162-74.ec2.internal    Ready     etcd      3m        v1.11.0+d4cacc0
ip-10-0-79-80.ec2.internal     Ready     master    3m        v1.11.0+d4cacc0
NAME      DESIRED   CURRENT   UP-TO-DATE   AVAILABLE   AGE
router    1         1         1            0           2s
error: timed out waiting for the condition
2018/08/01 18:15:41 Container setup in pod e2e-aws failed, exit code 1, reason Error
Another process exited

which is obviously unreleated to this change to an untested script.

/retest

@wking
Copy link
Member

wking commented Aug 1, 2018

This time the e2e-aws error was:

1 error(s) occurred:

* module.vpc.output.etcd_sg_id: Resource 'aws_security_group.etcd' does not have attribute 'id' for variable 'aws_security_group.etcd.*.id'

Dunno why e2e-aws has been so flaky recently...

/retest

@openshift-merge-robot openshift-merge-robot merged commit 1f451ef into openshift:master Aug 1, 2018
clnperez added a commit to clnperez/installer that referenced this pull request Dec 7, 2021
Fix openshift#78 for TF to operate correctly on stages
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.

7 participants