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

fix(core): Point users to the official documentation when they use n8n --help #8440

Merged

Conversation

despairblue
Copy link
Contributor

@despairblue despairblue commented Jan 25, 2024

Summary

The help users got with n8n --help was misleading. Some commands had no documentation. If a command had multiple subcommands, only the documentation for the first one would be shown in the main page, e.g. import would only mention that you can import credentials, but not that you can import workflows.

An idea here is that we can point the user directly to docs.n8n.io instead.

Downside may be that the docs will always document the latest version, but the user may run an older version of n8n. We're willing to accept that though. And the help for commands and topics would still be available.

This PR has no tests. I don't think it needs one, because the damage that is caused by the custom help message disappearing and the old one appearing again is limited.
If you disagree let me know and I write one, but right now it seams there are no tests that execute bin/n8n and check what it prints. So that would a completely new kind of test.

cli.help.mp4

Related tickets and issues

https://linear.app/n8n/issue/PAY-195/ensure-n8n-help-gives-useful-and-accurate-info

Review / Merge checklist

  • PR title and summary are descriptive. Remember, the title automatically goes into the changelog. Use (no-changelog) otherwise. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.

    A bug is not considered fixed, unless a test is added to prevent it from happening again.
    A feature is not complete without tests.

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Jan 25, 2024
@despairblue despairblue changed the title point users to the official documentation when they use n8n --help fix(core): Point users to the official documentation when they use n8n --help Jan 25, 2024
@despairblue despairblue changed the title fix(core): Point users to the official documentation when they use n8n --help fix(core): point users to the official documentation when they use n8n --help Jan 25, 2024
@despairblue despairblue changed the title fix(core): point users to the official documentation when they use n8n --help fix(core): Point users to the official documentation when they use n8n --help Jan 25, 2024
@despairblue despairblue marked this pull request as ready for review January 25, 2024 14:19
Copy link
Contributor

@krynble krynble left a comment

Choose a reason for hiding this comment

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

One thing I did not like is that now we don't have the help for specific commands anymore such as below.

I thought the top level n8n --help could benefit from taking people to the docs, but then special commands could still show their own help. WDYT?

Screenshot 2024-01-25 at 16 45 12

@despairblue
Copy link
Contributor Author

One thing I did not like is that now we don't have the help for specific commands anymore such as below.

I thought the top level n8n --help could benefit from taking people to the docs, but then special commands could still show their own help. WDYT?

From what I saw in the oclif docs that should be possible. Let me try.

@despairblue despairblue marked this pull request as draft January 25, 2024 17:34
@despairblue
Copy link
Contributor Author

@krynble it behaves the way you suggested.

This got me thinking now though.

We're hiding the CLI root help page because

  1. it was showing one command that should be hidden BaseCommand,
  2. some commands and topics were missing descriptions,
  3. some were showing the description of their first subcommand instead of a summary (import)

The idea is to send users from n8n --help to the online docs and from there they could potentially come back and try n8n db --help for example. That's not an ideal UX for people that explore the n8n CLI.

The benefit I saw from not maintaining the CLI help in the code was that we only have to maintain the CLI documentation in one place, that is https://docs.n8n.io/hosting/cli-commands/.

That benefit would have made up for the UX impairments. But now it seems we have both drawbacks

  1. impaired UX
  2. need to maintain the CLI docs in the code and online
    compared to just fixing the CLI root help page by hiding a command, documenting the others and finding out how topics can have documentation that is different from their subcommands.

Maybe we should merge this and create a ticket to bring the root help page back and fix it properly. WDYT?

@despairblue despairblue marked this pull request as ready for review January 25, 2024 20:44
@despairblue despairblue requested a review from krynble January 25, 2024 20:44
Copy link

cypress bot commented Jan 25, 2024

1 flaky test on run #3917 ↗︎

0 338 5 0 Flakiness 1

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 despairblue 🗃️ e2e/*
Project: n8n Commit: 277170e893
Status: Passed Duration: 20:11 💡
Started: Jan 26, 2024 7:18 AM Ended: Jan 26, 2024 7:38 AM
Flakiness  cypress/e2e/5-ndv.cy.ts • 1 flaky test

View Output Video

Test Artifacts
NDV > should not retrieve remote options when required params throw errors Test Replay Screenshots Video

Review all test suite changes for PR #8440 ↗︎

Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

Copy link
Contributor

✅ All Cypress E2E specs passed

@krynble
Copy link
Contributor

krynble commented Jan 26, 2024

@despairblue I agree, maybe the action item we can derive from this is revisit the docs page and see if it's up to date.

One thing I noticed is that the db:revert is missing from the docs, so we might also be missing others in there.

Could you then create another ticket to check whether our docs page is up to date?

@despairblue
Copy link
Contributor Author

Could you then create another ticket to check whether our docs page is up to date?

Will do. Should I assign it to payday or docs?

@krynble
Copy link
Contributor

krynble commented Jan 26, 2024

That should be Payday @despairblue - You can merge this PR then.

@despairblue despairblue merged commit 9f11eba into master Jan 26, 2024
30 checks passed
@despairblue despairblue deleted the pay-195-ensure-n8n-help-gives-useful-and-accurate-info branch January 26, 2024 14:16
@github-actions github-actions bot mentioned this pull request Jan 31, 2024
@janober
Copy link
Member

janober commented Feb 2, 2024

Got released with [email protected]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team Released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants