[Resolve #1451] Stop suppressing ClientError in describe_outputs

[alex-harvey-z3q]

This resolves a bug in sceptre outputs where all ClientError exceptions (e.g. ExpiredToken) are suppressed, for no apparent reason.

In 682a306, (October 2018, commit summary "Move Stack operations from Stack to StackActions"), a big refactor had occurred. That refactor had introduced a public method describe in the StackActions class.

A month later, in 182f0b6 (November 2018, commit summary "Implement algorithm to execute Stacks from StackGraph"), another big refactor occurred. That refactor introduced an essentially duplicate method _describe called by describe_output method, and then added code to catch all ClientError exceptions and return an empty list if one is raised.

So far as I can tell, there was no good reason to introduce either the new method, or catch all ClientError exceptions. The suppression of exceptions may have been accidental, a hack to get the unit tests to pass (or for a reason I have not thought of yet!).

Note that at the time of 182f0b6, the describe_outputs method that suppressed exceptions was not called anywhere other than in the CLI outputs commands:

% git co 182f0b64f1bfbb439c7e6f7778a3ce0d0774d187 
% grep -wr describe_outputs sceptre 
sceptre/plan/plan.py:    def describe_outputs(self, *args):
sceptre/plan/plan.py:        self._resolve(command=self.describe_outputs.__name__)
sceptre/plan/actions.py:    def describe_outputs(self):
sceptre/cli/list.py:        in plan.describe_outputs().values() if response

This patch attempts to fix all of this up:

• Duplicate method removed in favour of the public describe method. I have opted to keep the public method as the differ class is calling it, which means it should be public.

• The Nov 2018 change to describe_outputs which introduced the present bug is reverted.

• Duplicate logic to handle the case of "stack does not exist" removed.

• Tweak call to the describe method in the differ.

PR Checklist

• Wrote a good commit message & description [see guide below].
• Commit message starts with [Resolve #issue-number].
• Added/Updated unit tests.
• Added/Updated integration tests (if applicable).
• All unit tests (poetry run tox) are passing.
• Used the same coding conventions as the rest of the project.
• The new code passes pre-commit validations (poetry run pre-commit run --all-files).
• The PR relates to only one subject with a clear title.
  and description in grammatically correct, complete sentences.

Approver/Reviewer Checklist

• Before merge squash related commits.

Other Information

Guide to writing a good commit

[zaro0508]

Generally LGTM, however the integration tests are failing for this change.. https://github.com/Sceptre/sceptre/actions/runs/8405338399/job/23307454058

[alex-harvey-z3q]

Generally LGTM, however the integration tests are failing for this change.. https://github.com/Sceptre/sceptre/actions/runs/8405338399/job/23307454058

I can't remember if this is because I already fixed it or what but I can't see any failing pipelines. All good to merge this now?

[zaro0508]

@alexharv074 please take a look at my last comment, it contains a link to the integration test run.

[alex-harvey-z3q]

@zaro0508

@alexharv074 please take a look at my last comment, it contains a link to the integration test run.

I could be off base here but I do not think these 'describe stack' tests are needed and have tentatively deleted them. The scenarios like "the user describes resources in stack_group" refers to things that the user in fact cannot do and these are in effect integration tests on private methods. The tests were previously passing because the describe method called was swallowing up all exceptions and this is the bug this PR attempts to fix.

The second problem here is that keeping these tests would require us to now somehow test for exceptions being raised (remembering that these tests are really testing a private method and not something that the user can actually do). It looks like this is possible e.g. https://stackoverflow.com/questions/27894993/handling-exceptions-in-python-behave-testing-framework but would inevitably be a bit hacky.

I personally feel that these code paths are already tested by unit tests and therefore deleting these integration test cases could be acceptable.

[zaro0508]

Generally LGTM. Just seeing if there is an opportunity to do more cleanup?