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

auto generate localstack cli manual #812

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HarshCasper
Copy link
Member

No description provided.

@github-actions
Copy link

github-actions bot commented Sep 7, 2023

🎊 PR Preview has been successfully built and deployed to https://localstack-docs-preview-pr-812.surge.sh 🎊

Copy link
Contributor

@cabeaulac cabeaulac left a comment

Choose a reason for hiding this comment

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

Suggest not including deprecated commands daemons and infra. Since the CLI docs are just being re-added, let's not advertise commands we don't want to get used. ?

@@ -0,0 +1,54 @@
import subprocess
import re

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
black_list = ["infra", "daemons"]


# Loop through each command and its purpose
for cmd, cmd_purpose in command_purposes.items():
# Execute 'localstack [cmd] --help' and capture the output
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Execute 'localstack [cmd] --help' and capture the output
# Don't add commands from black list to the docs
if cmd in black_list:
continue
# Execute 'localstack [cmd] --help' and capture the output

Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

@cabeaulac: I don't think that we should remove deprecated functions from the docs. They are clearly marked as deprecated. A black list again is not automatically adjusted. What happens when we deprecate another operation in the next release?

@HarshCasper: The script looks great! However, it would be great if we could move this workflow to the localstack/localstack-cli repo, and trigger it on a published release (similar to the homebrew tap update). This is exactly when users can access the changes in the CLI, and we wouldn't have any time where the docs are mismatched again (because this workflow is only executed every three months).

@cabeaulac
Copy link
Contributor

cabeaulac commented Sep 8, 2023

@cabeaulac: I don't think that we should remove deprecated functions from the docs. They are clearly marked as deprecated. A black list again is not automatically adjusted. What happens when we deprecate another operation in the next release?

@HarshCasper: The script looks great! However, it would be great if we could move this workflow to the localstack/localstack-cli repo, and trigger it on a published release (similar to the homebrew tap update). This is exactly when users can access the changes in the CLI, and we wouldn't have any time where the docs are mismatched again (because this workflow is only executed every three months).

@alexrashed My point was

Since the CLI docs are just being re-added, let's not advertise commands we don't want to get used.

Otherwise, I totally agree, fine to have deprecated things in docs. But why advertise things we don't want used the first time we're advertising. Doesn't matter to me, was just trying to stop people from seeing these in docs and start using them.

Copy link
Member

@joe4dev joe4dev left a comment

Choose a reason for hiding this comment

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

The decision linking (see extensions PR) vs. duplicating through auto-generation is a bit trickier here due to the split of the source code in localstack, build in localstack-cli, and documentation currently in docs.

I see the value of auto-generating this manuals page upon new CLI releases (as @alexrashed suggested ideally triggered from localstack-cli).

I have a few suggestions and included the input from @MarcelStranak in this review.


This command shows the logs of the currently running LocalStack docker
container. By default, this command looks for a container named
`localstack_main` (which is the default container name used by the
Copy link
Member

Choose a reason for hiding this comment

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

changed to localstack-main (many other occurrences)

---
title: LocalStack CLI manual
weight: 3
description: The reference guide for LocalStack CLI commands and how to get started on using them!
Copy link
Member

Choose a reason for hiding this comment

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

ATM, it does not really show "how to get started on using them" (beyond the single completion example).

description: The reference guide for LocalStack CLI commands and how to get started on using them!
---

The LocalStack command-line interface (CLI) is a tool for starting, managing, and configuring your LocalStack container.
Copy link
Member

Choose a reason for hiding this comment

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

Related to @MarcelStranak 's point of missing the LocalStack CLI under the LocalStack Tools section discussed in #sig-docs here (internal link):

  • I agree with @MarcelStranak that under the current section structure, I'm also missing any mention of the LocalStack CLI under the Tools section, especially given that we define it as a "tool" right here. We need a landing page for the LocalStack CLI advertising why it should be used and highlighting the key features.
  • I agree with @HarshCasper that the installation instructions need to be under Getting Started. We could use a shared block to include in Getting Started and LocalStack CLI tools intro page.

💡 One idea is to have a short advertising quick starter guide under Tools and then link to this References page for full details.

@joe4dev
Copy link
Member

joe4dev commented Nov 17, 2023

@HarshCasper It would be good to fix the 404s related to the LocalStack CLI soon 🙏 :
Screenshot 2023-11-17 at 14 23 07

📆 IMHO, having at least some advertising page for the LocalStack CLI up for re:invent would be very helpful for users to evaluate whether that's something they should try.

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.

4 participants