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

Feature: implement get debug-info #778

Merged
merged 2 commits into from
May 29, 2019
Merged

Conversation

10ko
Copy link
Member

@10ko 10ko commented May 16, 2019

This PR introduces the command:

$ garden get debug-info

As spec'd out in #547, it's a command to collect all sort of system, project and providers information in a zip file. This will be useful data users can attach to github issues.

Thanks,
E.

@10ko 10ko force-pushed the feature-implement-get-debug-info branch 3 times, most recently from 10c688d to cdd4279 Compare May 16, 2019 16:37
@10ko 10ko changed the title Feature implement get debug info Feature: implement get debug-info May 16, 2019
@10ko 10ko marked this pull request as ready for review May 16, 2019 16:42
@10ko 10ko requested review from edvald, thsig and eysi09 May 16, 2019 16:42
Copy link
Collaborator

@edvald edvald left a comment

Choose a reason for hiding this comment

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

This is great! Just a bit of pedantry in my comments, except maybe that K8s namespace thing, which is possibly a bug. That aside, happy first feature! :D

docs/reference/commands.md Outdated Show resolved Hide resolved
garden-service/src/actions.ts Outdated Show resolved Hide resolved
garden-service/src/commands/get/get-debug-info.ts Outdated Show resolved Hide resolved
garden-service/src/commands/get/get-debug-info.ts Outdated Show resolved Hide resolved
garden-service/src/commands/get/get-debug-info.ts Outdated Show resolved Hide resolved
@eysi09
Copy link
Collaborator

eysi09 commented May 17, 2019

Had a few nitpicks of my own but otherwise looks great!

One thing we could improve though is the logging. E.g.: the body of the command action could look like this:

    const tempPath = join(garden.projectRoot, GARDEN_DIR_NAME, TEMP_DEBUG_ROOT)
    const entry = log.info({ msg: "Collecting debug info", status: "active" })
    const systemEntry = entry.info({ section: "system", msg: "collecting info", status: "active" })
    await collectBasicDebugInfo(garden.projectRoot, systemEntry)
    systemEntry.setSuccess({ msg: chalk.green(`Done (took ${systemEntry.getDuration(1)} sec)`), append: true })

    const providerEntry = entry.info({ section: "providers", msg: "collecting info", status: "active" })
    await collectProviderDebugInfo(garden, providerEntry, tempPath, opts.format)
    providerEntry.setSuccess({ msg: chalk.green(`Done (took ${providerEntry.getDuration(1)} sec)`), append: true })

    entry.setState("Preparing archive")
    const outputFilename = DEBUG_ZIP_FILENAME.replace("TIMESTAMP", new Date().toISOString())
    await zipFolder(tempPath, join(garden.projectRoot, outputFilename), log)

    await remove(tempPath)

    entry.setSuccess({ msg: "Done", append: true })
    log.info(`\nDone! Please find your report under ${garden.projectRoot}.`)

    return { result: 0 }

With this, we can simply remove the log statements from actions.getDebugInfo (and return the Bluebird map like @edvald suggested) and from collectSystemDiagnostic. The info statements that is, not the errors.

Passing the system and provider entries like this also ensures that the errors get set on the appropriate entry.

The end result looks something like this:

✔ Preparing archive → Done
   ✔ system                    → collecting project configuration files → Done (took 0.3 sec)
   ✔ providers                 → collecting info → Done (took 0.4 sec)

Done! Please find your report under /Users/eysi/code/garden-io/garden/examples/simple-project.

10ko added a commit that referenced this pull request May 22, 2019
@10ko 10ko force-pushed the feature-implement-get-debug-info branch from 5ab427c to 00a88ac Compare May 22, 2019 16:01
10ko added a commit that referenced this pull request May 22, 2019
Includes all change requests for PR #778 plus
fixes 404 on tmp-promise release 2.0.0 dependency
and some tests which broke after rebasing.
@10ko
Copy link
Member Author

10ko commented May 22, 2019

Dear all,
thanks a lot for the review, I agree on everything and wrote fixes and add-ons as you requested.

I have few notes and consideration to add:

  • @eysi09 The reason the command was running twice was because the garden project you tried the command against was:

    • a valid garden project, so
    • the normal command action would get called
    • at which point the command tried to collect the providers info but, more than likely, you had docker or k8s down.
    • the function collectProviderDebugInfo(...) would throw an exception.
    • The exception would then get caught in the cli.ts where generateBasicDebugInfoReport(...) would get called again:

    I added another try/catch in the function collecting the provider info.

  • The try/catch mentioned above will catch any exception thrown by any provider. Since the actions are executed through Bluebird.props(...): even if one of may providers fail, all (promises) will. It would be nice to catch only the failing providers. This would require a lot of internals work and was kinda out-of-scope for this PR. If any of you think it's something we want in the future I can open an issue.

  • @edvald I found a strange 404 error regarding a dependency called tmp-promise: the 2.0.0 version we used just got deleted from npm and github. I updated the dependecy to 2.0.1 but maybe we should investigate a replacement or if we need it at all since it doens't seem very reliable?

Thanks again and please let me know if you have any other comment.
E.

@eysi09
Copy link
Collaborator

eysi09 commented May 29, 2019

I believe the tmp-promise issue was resolved in another PR so rebasing should remove the need for that fix.

@eysi09
Copy link
Collaborator

eysi09 commented May 29, 2019

One thing I noticed after approving is that the debug info is not git ignored.

10ko and others added 2 commits May 29, 2019 12:19
Implements a utility command used to generate reports
containing information regarding the current project, OS and
providers. It generates a zip file which can be attached to
Github issues.

Co-authored-by: Thorarinn Sigurdsson <[email protected]>
Co-authored-by: Eyþór Magnússon <[email protected]>
Includes all change requests for PR #778 plus
fixes 404 on tmp-promise release 2.0.0 dependency
and some tests which broke after rebasing.
@eysi09 eysi09 force-pushed the feature-implement-get-debug-info branch from 00a88ac to 14063c0 Compare May 29, 2019 10:19
@eysi09 eysi09 merged commit 21d3602 into master May 29, 2019
@eysi09 eysi09 deleted the feature-implement-get-debug-info branch May 29, 2019 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants