Code review comment for lp:~viswesn/juju-ci-tools/ensure_provider_cleanup

> >> Whatever you do, don't repeat yourself. Factor out the duplicate code.
> <<Viswesn>> Currently we see most of the code is duplicated but then once we
> have list of resources to be cleaned up in coming days for each substrate then
> ensure_cleanup is going to be different for each substrate.

The parts that are the same, like terminate_instances, should not be duplicated. Factor them out.

> >> Don't catch Exception. Catch the specific exceptions that you expect to be
> raised.
> <<Viswesn>> Now I need to address this by sending the unclean=[] to the callee
> function and then append the resource name + exception something like
> ["instance_id", "instance-001", "Interface failed"]
>
> ------------------------------
> delete_detached_interfaces(self, security_group, unclean=None)
> if unclean is None:
> unclean=[]
> ...
> ....
> except Exception as ex:
> unclean.append(["instance_id", instance_name, type(ex).__name__,
> ex.args])
> ------------------------------
>
> Chris, let me know your comments on this. We decided not to do so in our last
> call but then to catch the exact exception then I need to pass unclean to the
> callee method rather than having like try..catch.. statement on invoking
> terminate_instance.

I don't understand why catching a specific exception would require you to pass unclean to the callee method.

> >> Are you certain that this script is not going to delete freshly-created
> resources?
> <<Viswesn>> if ensure_cleanup has visibility to those freshly-created resource
> then yes but seems like it is not at present and it is up the substrate
> developer to bring this in to picture as part of ensure_cleanup.

ensure_cleanup has visibility to those freshly-created resources. It is your problem, not the substrate developer's.