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

Adguard home instructions #517

Merged
merged 21 commits into from
Jan 24, 2025

Conversation

BrokenOnedroid
Copy link
Contributor

@BrokenOnedroid BrokenOnedroid commented Dec 27, 2024

To ease and accelerate the PR submission, please use the template below to provide all required information.
Please see our "Contributing to Rockstor documentation" in our Rockstor's documentation

Fixes #
If this is okay to use. The description in the PR can be linked to this Page. And then added to the rockon-registry. (See: rockstor/rockon-registry#397)

This pull request's proposal

Added write up for AdGuard Home Rock-on installation.

Checklist

  • With the proposed changes no Sphinx errors or warnings are generated.
  • I have added my name to the AUTHORS file, if required (descending alphabetical order).

@phillxnet
Copy link
Member

@BrokenOnedroid Thanks for seeing to this doc entry to complement your on-going Rock-on contributions ; much appreciated.

I've just run the Sphinx test build via this repos GitHub Action and we look to have an issue via the following Warning:

https://github.com/rockstor/rockstor-doc/actions/runs/12518381636/job/34940209577?pr=517#step:5:97

/home/runner/work/rockstor-doc/rockstor-doc/interface/docker-based-rock-ons/adguard-home.rst: WARNING: document isn't included in any toctree

Which suggests this pull requests branch is missing a toc (Table Of Content) entry for the page your have added. I.e. there is no menu entry generated for folks to find this new page.

Which in-turn ends-up being html rendered as:

Also note:

It would likely be better to use only one GitHub account here as it makes attribution easier. I suspect your local development environment is setup with the former: but you likely intend to use the later. We have a docs section with some details on configuring this:

Also keep in mind the second tick item: i.e. we like folks to get attribution for their submissions. But your choice of course.

Hope that helps.

@phillxnet
Copy link
Member

@BrokenOnedroid My apologies if I've jumped in too early here on this Draft status PR.

@BrokenOnedroid
Copy link
Contributor Author

@phillxnet thanks for pointing these things out.
I still need to check grammar and so on, its a wip.

I did not realize that my old username is used. I rename the github user sometime ago.
But i don't seem able to get the username in the commits changed. Any idea?

git_settings

@phillxnet
Copy link
Member

But i don't seem able to get the username in the commits changed. Any idea?

I think what is happening is that the username against old commits is not changed by altering the current config. But if you changed your local config and then did this latest commit then you likely also have to modify the associated email. GitHub I think uses the email to track commits. Note the other command in our docs e.g.:

git config user.email your_email_address

Plus upon this Pull Request reaching non-draft status, you can squashing all commits down to one under the new GitHub user and email. A submission will be required but that's not bother. I've taken to doing this myself quite a lot as it makes asking questions / opinions on draft work easier. And once all is ready one can then just squash and remove the remote (GitHub) branch and push again in the new squashed (single commit) status. That will then hopefully have the new user against the commit also.

Check your two GitHub accounts and compare the emails there-in. The PR is down OK as having been submitted by @BrokenOnedroid but the commits are all under that old account. So I'm guessing there is an email association that has precedence.

Feel free to experiment here in this PR as it will likely be squashed into a single commit and resubmitted later anyway given this is an example of a shared in-development branch: with the appropriate Draft status so we can all appreciate the in-development nature: as you say WIP. Looking nice though.

Hope that helps.

@BrokenOnedroid
Copy link
Contributor Author

From my pov i added everything necessary into the instructions.

make html does not produce errors.
make linkcheck shows errors, but these are HTTPSConnectionPool unrelated to my changes.

Copy link
Member

@Hooverdan96 Hooverdan96 left a comment

Choose a reason for hiding this comment

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

Content looks great! I suggested the Rock-on capitalization and a bit of semantic line-breaking that @phillxnet would like to move to (though I was probably not consistent in that). I am also suggesting the macvlan network creation using the WebUI System Shell.

interface/docker-based-rock-ons/adguard-home.rst Outdated Show resolved Hide resolved
interface/docker-based-rock-ons/adguard-home.rst Outdated Show resolved Hide resolved
interface/docker-based-rock-ons/adguard-home.rst Outdated Show resolved Hide resolved
interface/docker-based-rock-ons/adguard-home.rst Outdated Show resolved Hide resolved
interface/docker-based-rock-ons/adguard-home.rst Outdated Show resolved Hide resolved
interface/docker-based-rock-ons/adguard-home.rst Outdated Show resolved Hide resolved
interface/docker-based-rock-ons/adguard-home.rst Outdated Show resolved Hide resolved
interface/docker-based-rock-ons/adguard-home.rst Outdated Show resolved Hide resolved
interface/docker-based-rock-ons/adguard-home.rst Outdated Show resolved Hide resolved
interface/docker-based-rock-ons/adguard-home.rst Outdated Show resolved Hide resolved
@BrokenOnedroid
Copy link
Contributor Author

@Hooverdan96 all suggested changes are reasonable. So I committed all of them.

@Hooverdan96
Copy link
Member

@phillxnet, from my perspective this looks good. If you don't have any other concerns, I think this could be ready for a merge.

@phillxnet phillxnet marked this pull request as ready for review January 24, 2025 16:18
@phillxnet
Copy link
Member

@BrokenOnedroid Thanks for this super helpful Rock-on howto.

I'll merged & published, as per @Hooverdan96 recommendations, and to untangle our chicken & egg situation re your now ready for review PR of the associated Rock-on itself :).

We can always re-visit this doc entry for the trivial elements I've noted below.
No worries regarding this doc entry being published ahead of the associated Rock-on PR merge. I see it as the better way around anyway as reviewers of the Rock-on PR have the doc entry to follow during the review process.

Redundancy re link text:

Redundancy-re-link

In above we have repeat text once rendered.

Redundancy-re-link2

And here we can re-word to have the document word be our link, rather than the redundancy of "can be found here: ...". I.e. I think it's nicer to just have a link to the upstream docs, it may not even need it's own sub-section. But the note there re required config is really the key part. Any maybe we should lead with that and rename that subsection. But no worries as this can all be re-visited and the key element here is the requirement for this doc generally to enable the work you've done on the Rock-on.

Repeat word.

Navigate to to Access Control and click on Edit. "

@Hooverdan96 We should probably, in-time, try for some kind of automated check on PR's for spelling and repeat word errors such as this (and my own frequent typos of this sort). There will likely be additional plugins for Sphinx itself for this kind of thing.

Didn't want to hold up publishing just for these trivial typos/rewords given the inter-dependencies re the Rock-on itself.


I think it's also good to include, along with these by-hand configurations (a product of our fledgling Rock-ons subsystem) their equivalent undo procedures. I.e. in the case of this doc entry, a note on how to remove the added vlan for example.

@phillxnet phillxnet merged commit 62aa475 into rockstor:master Jan 24, 2025
3 checks passed
@phillxnet
Copy link
Member

PRODUCTION published

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