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

Print AppReport after dynamic ASG test for better debugging #612

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

dsabeti
Copy link
Contributor

@dsabeti dsabeti commented Oct 3, 2022

What is this change about?

This change adds an app report to the end of the dynamic ASG tests. If something goes wrong with the app, it's not currently possible to know why.

What version of cf-deployment have you run this cf-acceptance-test change against?

v21.8.0

Please check all that apply for this PR:

  • introduces a new test --- Are you sure everyone should be running this test?
  • changes an existing test
  • requires an update to a CATs integration-config

Did you update the README as appropriate for this change?

  • YES
  • N/A

If you are introducing a new acceptance test, what is your rationale for including it CATs rather than your own acceptance test suite?

N/A

How many more (or fewer) seconds of runtime will this change introduce to CATs?

This change adds a single call to cf logs --recent, so at most a few seconds.

What is the level of urgency for publishing this change?

  • Urgent - unblocks current or future work
  • Slightly Less than Urgent

Tag your pair, your PM, and/or team!

…le test

In some scenarios, CAPI returns an empty 'syslog_drain_url' field in addition to the credentials. The MatchJSON matcher is fairly strict -- if there are additional fields to what is specified, it will fail. This change makes the targeted assertion that the test actually cares about -- that the username is properly stored in the credentials.
…el test runs or runs on environments with littered service instances
… skipped

Because 'AssistedCredhubDescribe' and 'NonAssistedCredhubDescribe' were nested under the top-level 'CredhubDescribe', the BeforeEach of the top-level describe block would be run even when Assisted or NonAssisted tests were skipped. This removes the top-level CredhubDescribe and uses helper functions for the set up functionality shared by the two types of tests.
@ctlong
Copy link
Member

ctlong commented Oct 13, 2023

@dsabeti the CredhubDescribe function includes a similar BeforeEach to AssistedCredhubDescribe and NonAssistedCredhubDescribe that should skip any setup if neither "assisted" nor "non-assisted" are set.

Also, credhub/service_keys.go uses a similar pattern to credhub/service_bindings.go, so if we one is changed then I think the other should be changed as well.

If you think d1e1265 is worth it, could you please also apply similar changes to credhub/service_keys.go. Alternatively, if you remove d1e1265 from this PR, then I'm happy to merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Waiting for Changes
Development

Successfully merging this pull request may close these issues.

2 participants