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.

Sign up for GitHub

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

Jump to bottom

docs: tell how to deploy demo app in Hubble CLI guide #15973

Merged
merged 1 commit into from May 3, 2021
Merged

docs: tell how to deploy demo app in Hubble CLI guide #15973

merged 1 commit into from May 3, 2021

Conversation

lfundaro
Copy link
Contributor

@lfundaro lfundaro commented May 1, 2021

When following the guide on Inspecting the Network flow
with Hubble's CLI the Demo App is required to be deployed
but there is no reference to that anywhere following the
sequence of steps from the Getting Started guide.

Fixes: #15972

Signed-off-by: Lorenzo Fundaró lorenzofundaro@gmail.com

@lfundaro lfundaro requested a review from a team as a code owner May 1, 2021 21:16
@lfundaro lfundaro requested review from a team and qmonnet May 1, 2021 21:16
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 1, 2021
@lfundaro lfundaro requested a review from glibsm May 1, 2021 21:16
qmonnet
qmonnet requested changes May 1, 2021
View reviewed changes
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

I agree that there is some improvement needed. But I don't think that including the whole file for the instructions related to the SW demo is the best option:

  • It will occupy a good portion of the page, with detailed explanations on how the deployment works, when the current document really intends to get users started quickly with just a CLI tool.
  • It will not tell users how to generate traffic (By the way, the “Generate some network traffic” section relying on the connectivity tests integrates very badly with the rest of the page which relies on the SW demo, and should maybe be removed).
  • It will not tell users how to deploy network policies, so even if they generate traffic, it will not be marked as “dropped” as in the examples.

I would prefer one of the two:

  • Update the document to work with pods from the connectivity tests instead of the Star Wars demo, or
  • Instead of including the SW demo instructions, add a note mentioning that this page relies on the SW demo and that it assumes that the users have set up the demo + e.g. L7 policies, as described in ... [provide link, likely to this page).

Second option is easier and possibly the best one (makes it easier to follow what happens w.r.t. policies than connectivity tests). What do you think?

@qmonnet qmonnet changed the title Add instructions to deploy demo app docs: tell how to deploy demo app in Hubble CLI guide May 1, 2021
@qmonnet qmonnet added needs-backport/1.10 release-note/misc This PR makes changes that have no direct user impact. labels May 1, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 1, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.0-rc2 May 1, 2021
@qmonnet qmonnet added the area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. label May 1, 2021
@lfundaro
Copy link
Contributor Author

lfundaro commented May 2, 2021 •
edited

But I don't think that including the whole file for the instructions related to the SW demo is the best option

Yeah, I also feel like it will diverge people's attention out of this guide.

I'm proposing adding a note before the guide starts, check it out and let me know what you think. Cheers !

@lfundaro lfundaro requested review from a team and jrajahalme May 2, 2021 18:46
@lfundaro lfundaro requested a review from qmonnet May 2, 2021 18:47
qmonnet
qmonnet requested changes May 2, 2021
View reviewed changes
Documentation/gettingstarted/hubble_cli.rst Outdated
.. note::

Before you begin, make sure you have deployed the Demo Application together with
its network policies. Please refer to :ref:`gs_http_sw_demo` for more details.
Copy link
Member

Choose a reason for hiding this comment

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

Can we phrase this differently please? More “the examples below use the Demo App presented at...” than “Make sure to deploy this”. This tutorial provides examples on how to use the command line, but it's not a step-by-step tutorial to unroll some scenario-based use case, so I think we shouldn't push the reader to install it, just let them know (Sorry, I'm picky :) ).

Also I'd just link to _gs_http, not sure it's worth creating a new target below the requirements. But I don't mind much, so this is your call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Sorry, I'm picky :)

hahaha, no worries, this type of thoughtful discussions is what makes open source great 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

one thing though, you do need to have the Demo App deployed to run these examples right ? So, maybe one can say:

    This guide uses examples based on the Demo App. If you would like to run them,
    you would need to deploy the Demo App first. Please refer to :ref:`gs_http`
    for more details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed something based on this previous comment, I'm always open to discuss if you still think the wording doesn't match the intent. Cheers !

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me, thank you for the change!

Documentation/gettingstarted/hubble_cli.rst Outdated
@@ -33,7 +37,7 @@ In order to generate some network traffic, run the connectivity test:

.. code-block:: shell-session

while true; do cilium connectivity test; done
while true; do cilium connectivity test; done
Copy link
Member

Choose a reason for hiding this comment

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

I think this whole subsection should be gone. The Star Wars demo does not rely on the connectivity tests.

@maintainer-s-little-helper
Copy link

Commits b8c8f347784c1d7f5a0cc871e15c5913a4014f26, d630c39e28ccb60d644177d98738345cad942bd6 do not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 2, 2021
@lfundaro lfundaro force-pushed the ref_to_demo_in_hubble_exercise branch from d630c39 to 8ebacfc Compare May 2, 2021 20:56
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 2, 2021
@lfundaro lfundaro requested review from qmonnet and removed request for a team May 2, 2021 20:57
qmonnet
qmonnet approved these changes May 2, 2021
View reviewed changes
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks a lot! Just one more thing from me, could you please squash your commits into a single one?

@lfundaro lfundaro force-pushed the ref_to_demo_in_hubble_exercise branch from 8ebacfc to 40cee97 Compare May 3, 2021 07:10
@lfundaro
Copy link
Contributor Author

lfundaro commented May 3, 2021

Looks good to me, thanks a lot! Just one more thing from me, could you please squash your commits into a single one?

sure, done 👍 . I also updated the commit message to better match the intent of this PR.
Cheers !

rolinh
rolinh approved these changes May 3, 2021
View reviewed changes
Copy link
Member

@rolinh rolinh left a comment •
edited

Choose a reason for hiding this comment

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

Thanks! Minor suggestion below. And as @qmonnet asked, could you please squash your commits?

EDIT: there was a race posting a comment to this issue 😀

Documentation/gettingstarted/hubble_cli.rst Outdated Show resolved Hide resolved
@maintainer-s-little-helper
Copy link

Commit 9584fa18e984dd355b4188af93018e53817f85dc does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 3, 2021
@lfundaro
Add missing Demo App reference
7eda947 
When following the guide on Inspecting the Network flow
with Hubble's CLI there are many examples that are based
on the Demo App. This PR adds a note to take users to
the Demo App guide so they can deploy it in case they
would like to run the examples of this guide

Fixes: cilium#15972

Signed-off-by: Lorenzo Fundaró <lorenzofundaro@gmail.com>
@lfundaro lfundaro force-pushed the ref_to_demo_in_hubble_exercise branch from 9584fa1 to 7eda947 Compare May 3, 2021 07:16
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 3, 2021
@lfundaro lfundaro requested a review from rolinh May 3, 2021 07:17
@rolinh rolinh added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 3, 2021
@rolinh rolinh merged commit 1070b19 into cilium:master May 3, 2021
@lfundaro lfundaro deleted the ref_to_demo_in_hubble_exercise branch May 3, 2021 10:26
@brb brb mentioned this pull request May 7, 2021
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.0-rc2 May 7, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.0-rc2 May 13, 2021
@aanm aanm mentioned this pull request May 17, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rolinh rolinh rolinh approved these changes

@qmonnet qmonnet qmonnet approved these changes

@glibsm glibsm Awaiting requested review from glibsm

@jrajahalme jrajahalme Awaiting requested review from jrajahalme

Assignees

@glibsm glibsm

@rolinh rolinh

@jrajahalme jrajahalme

@qmonnet qmonnet

Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.10.0-rc2
Backport done to v1.10
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing reference to demo app in getting started
7 participants
@lfundaro @rolinh @qmonnet @brb @glibsm @joestringer @jrajahalme