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

Update network_increase_bandwidth.md document with details for u18.04+ #13457

Merged
merged 11 commits into from
May 17, 2024

Conversation

JohnHammell
Copy link
Contributor

The current document does not work properly for Ubuntu 22.04 and probably any version >= 18.04.

I updated this page to include details on how to do this for versions 18.04+ and kept the original section, renaming that to be for versions <= 17.04.

Also added an example command at the bottom on how to change the queue.tx.length in the default LXD profile so that change would apply to all containers.

The current document does not work properly for Ubuntu 22.04 and probably any version >= 18.04.

I updated this page to include details on how to do this for versions 18.04+ and kept the original section, renaming that to be for versions <= 17.04.

Also added an example command at the bottom on how to change the queue.tx.length in the default LXD profile so that change would apply to all containers.

Signed-off-by: JohnHammell <git@johnhammell.com>
@github-actions github-actions bot added the Documentation Documentation needs updating label May 8, 2024
Copy link

github-actions bot commented May 8, 2024

Heads up @ru-fu - the "Documentation" label was applied to this issue.

@ru-fu
Copy link
Contributor

ru-fu commented May 8, 2024

Thanks a lot @JohnHammell !

Can you please sign the CLA so we can get this change in? See here: https://ubuntu.com/legal/contributors

@JohnHammell
Copy link
Contributor Author

@ru-fu thanks for pointing that out and linking to it. I signed / submitted the CLA a few minutes ago.

@JohnHammell
Copy link
Contributor Author

@ru-fu Is there anything I need to do in order to re-run and/or correct the failing checks for this pull request? Or is it currently fine as-is? I'm a bit new to submitting pull requests to Canonical, so wanted to make sure I'm doing this correctly. Thanks.

@ru-fu
Copy link
Contributor

ru-fu commented May 12, 2024

@ru-fu Is there anything I need to do in order to re-run and/or correct the failing checks for this pull request? Or is it currently fine as-is? I'm a bit new to submitting pull requests to Canonical, so wanted to make sure I'm doing this correctly. Thanks.

Hi @JohnHammell - I re-ran the action, and it seems to be an issue with the check right now. If you signed the CLA with your GitHub user name (and the correct capitalization), you should be all set and the problem is on our side. I'll check. :)

@JohnHammell
Copy link
Contributor Author

@ru-fu Am I supposed to fix things with this pull request until all checks pass, before it can be reviewed?

If so, how do I trigger the checks to re-run after making corrections?

The one check failing (Commits / Branch target and CLA) states no launchpad account. However, I do have a launchpad account and did include that in the CLA form as it cannot be submitted without the launchpad account part. Does my email used on Github need to match my email address on Launchpad? The CLA did not require that they match, the form was able to submit with them being different, but the error message in the check seems to be looking for a Launchpad account that matches my email address used on Github rather than the Launchpad account that I specified on the CLA.

The other check that's failing (Tests / Documentation), in the details it mentions 2 lines with trailing spaces and another line fails with the message "Ordered list item prefix." Do I need to "Convert to draft" to make those corrections?

Copy link
Contributor

@ru-fu ru-fu left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good - I added a few suggestions (mainly to make the spell checker happy ;) ).

I think this would work better using tabs for the different Ubuntu versions, instead of a section for each. Do you want to give this a try? But I also don't mind getting it in as is and changing it later.

doc/howto/network_increase_bandwidth.md Outdated Show resolved Hide resolved
doc/howto/network_increase_bandwidth.md Outdated Show resolved Hide resolved
doc/howto/network_increase_bandwidth.md Outdated Show resolved Hide resolved
doc/howto/network_increase_bandwidth.md Outdated Show resolved Hide resolved
doc/howto/network_increase_bandwidth.md Outdated Show resolved Hide resolved
@ru-fu
Copy link
Contributor

ru-fu commented May 13, 2024

The one check failing (Commits / Branch target and CLA) states no launchpad account. However, I do have a launchpad account and did include that in the CLA form as it cannot be submitted without the launchpad account part. Does my email used on Github need to match my email address on Launchpad? The CLA did not require that they match, the form was able to submit with them being different, but the error message in the check seems to be looking for a Launchpad account that matches my email address used on Github rather than the Launchpad account that I specified on the CLA.

The check requires either GitHub or Launchpad, so only one of them is sufficient. :)
The issue was on our side but seems to be resolved, it's green now.

The other check that's failing (Tests / Documentation), in the details it mentions 2 lines with trailing spaces and another line fails with the message "Ordered list item prefix." Do I need to "Convert to draft" to make those corrections?

No need for that - the checks will run again when you update the PR with your changes.

JohnHammell and others added 6 commits May 13, 2024 18:17
Co-authored-by: Ruth Fuchss <ruth.fuchss@canonical.com>
Signed-off-by: JohnHammell <git@johnhammell.com>
Co-authored-by: Ruth Fuchss <ruth.fuchss@canonical.com>
Signed-off-by: JohnHammell <git@johnhammell.com>
Co-authored-by: Ruth Fuchss <ruth.fuchss@canonical.com>
Signed-off-by: JohnHammell <git@johnhammell.com>
Co-authored-by: Ruth Fuchss <ruth.fuchss@canonical.com>
Signed-off-by: JohnHammell <git@johnhammell.com>
Removed 'sudo' from in front of 'sudo lxc' command.

Signed-off-by: JohnHammell <git@johnhammell.com>
Changed the two sections for Ubuntu >= 18.04 and Ubuntu <= 17.04 into tabs as suggested by @ru-fu

Signed-off-by: JohnHammell <git@johnhammell.com>
@JohnHammell
Copy link
Contributor Author

JohnHammell commented May 13, 2024

Thanks! This looks good - I added a few suggestions (mainly to make the spell checker happy ;) ).

Thanks for all the suggestions, I added them in.

I think this would work better using tabs for the different Ubuntu versions, instead of a section for each. Do you want to give this a try? But I also don't mind getting it in as is and changing it later.

I gave it a try and think I may have it correct, but not certain as the code editor preview doesn't seem to be able to display tabbed sections. Also couldn't seem to find anything about this type of tab in the github docs. What I ended up doing was just found another page that used tabs & replicated how that was done. Hopefully I got it right.

Also, the verification checks above, the 3 that failed had a message stating "The user has cancelled this build." I don't recall cancelling any builds. Not quite sure why that happened. Maybe they were automatically cancelled when I submitted the other changes, not sure as it didn't say that in the message.

@JohnHammell JohnHammell requested a review from ru-fu May 14, 2024 00:05
Copy link
Contributor

@ru-fu ru-fu left a comment

Choose a reason for hiding this comment

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

Thanks a lot, this looks great! Sorry for a typo, that was my fault.

You can look at the output here:
https://canonical-ubuntu-documentation-library--13457.com.readthedocs.build/lxd/en/13457/howto/network_increase_bandwidth/#increase-the-network-bandwidth-on-the-lxd-host

There are some more linting problems (see the doc check). If you want to run the linter locally, you can use make doc-lint

doc/howto/network_increase_bandwidth.md Outdated Show resolved Hide resolved
Co-authored-by: Ruth Fuchss <ruth.fuchss@canonical.com>
Signed-off-by: JohnHammell <git@johnhammell.com>
@JohnHammell
Copy link
Contributor Author

@ru-fu That link for reviewing the output, where did you get that from? I didn't see that anywhere for previewing changes. Would be good to know for future edits that I do.

Also, for the make doc-lint , I would like to try that, but can you point me in the right direction? I'm a bit rusty with git and submitted this pull request via the website, through the edit this document link on the page we are editing.

I setup git, gh & all that. But still not sure where to go to do the make doc-lint , I can make doc but not make doc-lint in the repo.

If there's a how-to page describing how to do all these things, please post a link to that and I'll see if I can figure things out better from that.

@ru-fu
Copy link
Contributor

ru-fu commented May 15, 2024

@JohnHammell You can get to the doc preview if you look at the checks at the bottom of the PR and click the "Details" link for the Read the Docs build:
image

It's quite hidden, but very convenient once you know about it. :)

See here for a short intro on how to work with the documentation: https://documentation.ubuntu.com/lxd/en/latest/contributing/#contribute-to-the-documentation

If you are working locally, make doc-lint should run in the same way as make doc. What kind of error are you getting?
But it's also totally fine to use the web UI and have the check run on the PR. It might just take a few more iterations sometimes. :)

Copy link
Contributor

@ru-fu ru-fu left a comment

Choose a reason for hiding this comment

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

I added some suggestions that should fix the linting errors. 🤞

doc/howto/network_increase_bandwidth.md Outdated Show resolved Hide resolved
doc/howto/network_increase_bandwidth.md Outdated Show resolved Hide resolved
doc/howto/network_increase_bandwidth.md Show resolved Hide resolved
JohnHammell and others added 3 commits May 17, 2024 00:15
Co-authored-by: Ruth Fuchss <ruth.fuchss@canonical.com>
Signed-off-by: JohnHammell <git@johnhammell.com>
Co-authored-by: Ruth Fuchss <ruth.fuchss@canonical.com>
Signed-off-by: JohnHammell <git@johnhammell.com>
Co-authored-by: Ruth Fuchss <ruth.fuchss@canonical.com>
Signed-off-by: JohnHammell <git@johnhammell.com>
@JohnHammell
Copy link
Contributor Author

@ru-fu That doc preview link is definitely hard to find, but glad that I know where to find it now. It's quite useful for document change previews and probably should be more visible. Maybe even have that in place of the preview area with the code editor as that preview wasn't very good/accurate.

I'll get back to the git cli stuff later when I have a chance, for the remainder of this PR I added in those other changes you suggested via the web UI. So I think we are probably all set for now.

Let me know if there's anything else we need to do.

Thanks for all your help and giving me the chance to learn a bit more github stuff while working on this.

@JohnHammell JohnHammell requested a review from ru-fu May 17, 2024 06:33
Copy link
Contributor

@ru-fu ru-fu left a comment

Choose a reason for hiding this comment

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

Thank you, this looks good now!

Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

Thanks!

@tomponline tomponline merged commit affed4e into canonical:main May 17, 2024
28 checks passed
@JohnHammell JohnHammell deleted the patch-2 branch May 18, 2024 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Documentation needs updating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants