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

feat(cli): Include provider info in cdktf debug #3190

Merged
merged 2 commits into from
Nov 1, 2023

Conversation

Maed223
Copy link
Contributor

@Maed223 Maed223 commented Oct 15, 2023

Related issue

Fixes #2190

Description

Includes provider info (local and prebuilt) into cdktf debug.

cdktf debug

Screenshot 2023-10-16 at 5 53 57 PM

cdktf debug --json

Screenshot 2023-10-16 at 5 54 16 PM

Checklist

  • I have updated the PR title to match CDKTF's style guide
  • I have run the linter on my code locally
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation if applicable
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works if applicable
  • New and existing unit tests pass locally with my changes

Copy link
Contributor

@DanielMSchmidt DanielMSchmidt left a comment

Choose a reason for hiding this comment

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

While I can see that you would want to re-use the existing list logic this solution is a bit unclean.

  1. it uses the handler function providerList directly, which is intended to be hooked into yargs, which is not ideal
  2. it changes the return value of this function from void to void | Provider[] (not sure of the correct type of allProviders, but you get the idea). That's not ideal since it's now unclear what is returned, a caller would need to guess or look into the function, leading to an implicit contract being formed here
  3. there is already a nice abstraction that gives you all providers: manager.allProviders(), you could use that
  4. there is a central function to collect all debug information, it would be nice if everything print in debug is fetched in collectDebugInformation.
  5. IMHO it does not need to be as pretty as the table we normally sent for debugging, having a new line seperated list of - hashicorp/[email protected] (pre-built, version 5.3.3) would be good enough

@DanielMSchmidt DanielMSchmidt added this pull request to the merge queue Nov 1, 2023
Merged via the queue into main with commit a421997 Nov 1, 2023
21 checks passed
@DanielMSchmidt DanielMSchmidt deleted the debug-list-providers branch November 1, 2023 08:35
@Maed223 Maed223 mentioned this pull request Nov 30, 2023
Copy link
Contributor

github-actions bot commented Dec 2, 2023

I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cdktf debug should list prebuilt provider versions
3 participants