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

Adding connectivity test to troubleshooting doc #11643

Merged
merged 4 commits into from Jun 2, 2020
Merged

Adding connectivity test to troubleshooting doc #11643

merged 4 commits into from Jun 2, 2020

Conversation

jedsalazar
Copy link
Contributor

@jedsalazar jedsalazar commented May 21, 2020 •
edited by aanm

Previously the troubleshooting documentation lacked a reference to the connectivity tests specified in the getting started guide.

Added the connectivity guide reference as well as some minor style updates

Fixes: #11581

Signed-off-by: Jed Salazar jed@isovalent.com

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Thanks for contributing!

Fixes: #issue-number

Add connectivity test to troubleshooting

@jedsalazar jedsalazar requested a review from a team as a code owner May 21, 2020 17:48
@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 May 21, 2020
@jedsalazar jedsalazar added the release-note/misc This PR makes changes that have no direct user impact. label May 21, 2020
@michi-covalent
Copy link
Contributor

@jedsalazar looks like you need to update the spelling word list.

you can also run make render-docs locally to see if there are any syntax/spelling errors.

https://docs.cilium.io/en/latest/contributing/testing/documentation/

@coveralls
Copy link

coveralls commented May 21, 2020 •
edited

Coverage Status

Coverage decreased (-0.04%) to 36.886% when pulling ec22fa4 on jedsalazar:add-connectiviy-test-to-troubleshooting-doc into 60bffa4 on cilium:master.

@tgraf tgraf added the area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. label May 21, 2020
@maintainer-s-little-helper
Copy link

Commit 33d89c40cb6bb80a2c152f9cbb5bf5974032b2e3 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 21, 2020
@netlify
Copy link

netlify bot commented May 21, 2020 •
edited

Deploy preview for cilium-docs failed.

Built with commit 33d89c40cb6bb80a2c152f9cbb5bf5974032b2e3

https://app.netlify.com/sites/cilium-docs/deploys/5ec6cbc594e7af00079df10e

Adding connectivity test to troubleshooting doc
675797a 
Previously the troubleshooting documentation lacked a reference to the connectivity tests specified in the getting started guide.

Added the connectivity guide reference as well as some minor style updates

Fixes: cilium#11581

Signed-off-by: Jed Salazar <jed@isovalent.com>

Updating issues in the docs

Fixing ts_agent_logs line, adding a link to the test and fixing a spelling error
@jedsalazar jedsalazar force-pushed the add-connectiviy-test-to-troubleshooting-doc branch from 33d89c4 to 675797a Compare May 21, 2020 19:33
@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 21, 2020
joestringer
joestringer requested changes May 21, 2020
View reviewed changes
Copy link
Member

@joestringer joestringer 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 a welcome change :)

Couple of comments below.

Documentation/troubleshooting.rst Show resolved Hide resolved
michi-covalent
michi-covalent reviewed May 21, 2020
View reviewed changes
Copy link
Contributor

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

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

lgtm overall, one minor comment about not hardcoding the release tag.

Documentation/troubleshooting.rst Outdated
variant and the readiness and liveness gate indicates success or failure of the
test:

.. _test: https://raw.githubusercontent.com/cilium/cilium/1.7.4/examples/kubernetes/connectivity-check/connectivity-check.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

i think you need to use the SCM_WEB variable defined in Documentation/conf.py here to avoid hardcoding branches/tags. here is an example: https://github.com/cilium/cilium/blame/054dd163fdb792c44b3c493b8d6fd111e5e1a2bc/Documentation/gettingstarted/dns.rst#L33

@maintainer-s-little-helper
Copy link

Commit d4ae623733662b7e504d201be61c8fba9074c353 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 21, 2020
jedsalazar
jedsalazar commented May 21, 2020
View reviewed changes
Copy link
Contributor Author

@jedsalazar jedsalazar left a comment

Choose a reason for hiding this comment

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

lgtm overall, one minor comment about not hardcoding the release tag.

thanks, Michi! added. it doesn't seem to render on my local build. going see if it works in netlify

Documentation/troubleshooting.rst Show resolved Hide resolved
@maintainer-s-little-helper
Copy link

Commit d4ae623733662b7e504d201be61c8fba9074c353 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

1 similar comment
@maintainer-s-little-helper
Copy link

Commit d4ae623733662b7e504d201be61c8fba9074c353 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

@aanm
Copy link
Member

aanm commented May 27, 2020

@jedsalazar some commits are missing sign-offs

jedsalazar added 3 commits May 27, 2020 08:00
Update link to reference SCM_WEB var
18e6fa7 
Hard-coding is bad, variables are good.

Fixes: cilium#11581

Signed-off-by: Jed Salazar <jed@isovalent.com>
Adding cilium-test namespace creation and depth of test descriptions
1b171e7 
Given that the connectivity testing is referenced in the troubleshooting docs, it's worth deploying them to an empty namespace, `cilium-test`. h/t @joe

Also included an initial effort to inlcude a table describing the coverage that each testing type provides.

Fixes: cilium#11581

Signed-off-by: Jed Salazar <jed@isovalent.com>
Fix spelling error and add intra to wordlist
ec22fa4 
Fixes: cilium#11581

Signed-off-by: Jed Salazar <jed@isovalent.com>
@jedsalazar jedsalazar force-pushed the add-connectiviy-test-to-troubleshooting-doc branch from a3b97c3 to ec22fa4 Compare May 27, 2020 14:02
@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 27, 2020
@jedsalazar
Copy link
Contributor Author

@jedsalazar some commits are missing sign-offs

Thanks, @aanm. I updated the commits. PTAL

@aanm aanm requested a review from joestringer May 29, 2020 15:01
@aanm aanm requested a review from michi-covalent May 29, 2020 15:01
michi-covalent
michi-covalent approved these changes May 29, 2020
View reviewed changes
Copy link
Contributor

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

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

let's roll

@joestringer joestringer merged commit 3da5bcf into cilium:master Jun 2, 2020
1.8.0 automation moved this from In progress to Merged Jun 2, 2020
@tklauser tklauser mentioned this pull request Jun 3, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer approved these changes

@michi-covalent michi-covalent michi-covalent approved these changes

Assignees
No one assigned
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.0
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Docs: Add connectivity-check.yaml instructions Cilium troubleshooting section
7 participants
@jedsalazar @michi-covalent @coveralls @aanm @joestringer @tklauser @tgraf