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(docs): Clarify quick start installation. Fixes #13032 #13047

Merged
merged 6 commits into from
May 16, 2024

Conversation

mfisher87
Copy link
Contributor

I got hung up on these instructions because I copied the commands from the releases page instead of from the quick start page, so I applied install.yaml instead of quick-start-minimal.yaml. I hope this change will help others not get confused like me :)

I tried make pre-commit -B (had to install go first) and it failed with protoc not found. Do I need to keep working my way through these, or is it OK to ignore for a docs change like this?

Fixes #13032

Motivation

I followed the quick start instructions explicitly, but ended up with an install that couldn't execute Hello World step.

Modifications

Alter docs to explicitly instruct the user to install the quick start manifest.

I also moved a warning admonition because I felt it belonged after prose I added in this PR.

Verification

Rendered the docs, looked good

@mfisher87 mfisher87 changed the title Clarify the quick start instructions. Fixes #13032 fix(docs): Clarify the quick start instructions. Fixes #13032 May 14, 2024
@mfisher87 mfisher87 force-pushed the clarify-quickstart branch 2 times, most recently from 59f99c5 to 94f1316 Compare May 14, 2024 01:20
@mfisher87
Copy link
Contributor Author

Apologies, knew I should have opened as a draft but clicked too hastily and missed the little arrow on the button.

@mfisher87 mfisher87 marked this pull request as draft May 14, 2024 01:21
@mfisher87 mfisher87 marked this pull request as ready for review May 14, 2024 01:27
@agilgur5 agilgur5 self-assigned this May 14, 2024
@agilgur5 agilgur5 added the area/docs Incorrect, missing, or mistakes in docs label May 14, 2024
@agilgur5 agilgur5 changed the title fix(docs): Clarify the quick start instructions. Fixes #13032 fix(docs): Clarify quick start installation. Fixes #13032 May 14, 2024
@agilgur5
Copy link
Member

I tried make pre-commit -B (had to install go first) and it failed with protoc not found. Do I need to keep working my way through these, or is it OK to ignore for a docs change like this?

It's not necessary for most of the docs, only the codegen'd ones. CI passes so you're good.
Also, the Makefile defines protoc 🤔

@mfisher87
Copy link
Contributor Author

Great, thanks for confirming! Happy to make any changes or rebase or whatever :)

@agilgur5
Copy link
Member

agilgur5 commented May 14, 2024

Yea I was gonna request some small changes when I was at my computer (previous comments were from my phone). Should be done later today

@mfisher87
Copy link
Contributor Author

Thanks for your time, Anton ❤️

Just want to say I'm really impressed with the community y'all have built here 🤩

Copy link
Member

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

A few small changes below

docs/quick-start.md Outdated Show resolved Hide resolved
docs/quick-start.md Outdated Show resolved Hide resolved
docs/quick-start.md Outdated Show resolved Hide resolved
mfisher87 and others added 5 commits May 14, 2024 16:12
I got hung up on these because I copied the commands from the releases
page instead of from the quick start page, so I accidentally applied
`install.yaml` instead of `quick-start-minimal.yaml`. I hope this change
will help others not get confused like me :)

Signed-off-by: Matt Fisher <[email protected]>
Co-authored-by: Anton Gilgur <[email protected]>
Signed-off-by: Matt Fisher <[email protected]>
Signed-off-by: Matt Fisher <[email protected]>
docs/quick-start.md Outdated Show resolved Hide resolved
@mfisher87 mfisher87 requested a review from agilgur5 May 14, 2024 22:30
Signed-off-by: Anton Gilgur <[email protected]>
Copy link
Member

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for pointing this out and fixing it!

@agilgur5 agilgur5 enabled auto-merge (squash) May 16, 2024 00:42
@agilgur5 agilgur5 merged commit 9f620d7 into argoproj:main May 16, 2024
16 checks passed
@mfisher87 mfisher87 deleted the clarify-quickstart branch May 16, 2024 00:49
@agilgur5 agilgur5 added this to the v3.5.x patches milestone May 16, 2024
agilgur5 added a commit that referenced this pull request May 27, 2024
Signed-off-by: Matt Fisher <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
Co-authored-by: Anton Gilgur <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs Incorrect, missing, or mistakes in docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs bug: following the quick-start guide _too_ explicitly can get users in trouble
2 participants