-
Notifications
You must be signed in to change notification settings - Fork 75
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
docs: add anbox documentation #2695
Conversation
| realtime-kernel | ✅ | ✅ | ❌ | ✅| ✅| ✅ | ❌ | ❌ | N/A | ✅ | ✅ | ✅ | | ||
| ros | ✅ | ✅ | ✅ | ✅| ✅| ✅ | ✅ | ✅ | ✅ | N/A | ✅ | ✅ | | ||
| ros-updates | ✅ | ✅ | ✅ | ✅| ✅| ✅ | ✅ | ✅ | ✅ | ✅ | N/A | ✅ | | ||
| anbox-cloud | ✅ | ✅ | ✅ | ✅| ✅| ✅ | ✅ | ✅ | ✅ | ✅ | ✅ | ✅ | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@s-makin I don't like how the table is presented after this new change. Let me know if you have a better idea on how to address that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had the same concerns last time and now it's come back to bite us :/ I've been digging, and I think the only way we'll be able to display this table sensibly (e.g., with vertical top headers) is by using html and css. We could use tabs, to only show one column at a time, but that would be weird. Although if we followed the grouping on the front page, that might work? The columns could then just be the columns applicable to a specific service need, and it wouldn't matter if we showed all rows for each. We might need to have a chat about it later, let me know what you think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree we should chat about this for sure
@s-makin I have updated the header text to be vertically align on the compatibility matrix table |
ff79416
to
6e4ffbe
Compare
@lucasmoura I think this PR needs to be rebased on |
Yeah, the table looks really nice now but I was confused about all the extra stuff that suddenly appeared |
6e4ffbe
to
60c43fd
Compare
@s-makin @orndorffgrant fixed |
@lucasmoura The livepatch row, fips-updates column needs to be updated to a green check. Also I'm experimenting with a less repetitive way of displaying the table, hopefully posting another comment soon |
@s-makin @lucasmoura I came up with this but I'm not actually convinced its easier to read so feel free to ignore. |
docs/howtoguides/enable_anbox.md
Outdated
which will install all of the necessary snaps and setup the APT sources needed for | ||
the service. | ||
|
||
```{important} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we combine this admonition with the one at the top of the page? It really confused me to see two of them so close together right at the start of the guide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it to another part of the doc. Let me know what you think
configuration changes. | ||
For more information, see https://anbox-cloud.io/docs/tut/installing-appliance | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to include anything about disabling/removing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added that section
60c43fd
to
edb0fe7
Compare
@s-makin Do you think Grant's table suggestion is better than what we currently have ? |
sudo pro disable anbox-cloud | ||
``` | ||
|
||
Note that this command will only remove the APT sources, but not uninstall the snaps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like for a useability pov that it should also remove the snaps. Is that something we have control over, or is it for the Anbox team?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually, we don't remove any packages/snaps that we install when enabling a service. Anbox is just following that principle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on this. I'm happy with how the matrix is rendering for now, so let's not hold this PR up any longer fiddling with it.
I've found the final round of nits, and one question, but I'll drop an approving review anyway since I won't be here til next Thursday. Once you've taken care of the nits, please feel free to merge :)
docs/howtoguides/enable_anbox.md
Outdated
``` | ||
|
||
To use Anbox, you will need to enable it directly throught the Ubuntu Pro Client, | ||
which will install all of the necessary snaps and setup the APT sources needed for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which will install all of the necessary snaps and setup the APT sources needed for | |
which will install all of the necessary snaps and set up the APT sources needed for |
docs/howtoguides/enable_anbox.md
Outdated
``` | ||
|
||
```{important} | ||
The Anbox Cloud service can only be installed on containers using the --access-only flag. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Anbox Cloud service can only be installed on containers using the --access-only flag. | |
The Anbox Cloud service can only be installed on containers using the `--access-only` flag. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(to make sure the --
characters show correctly)
docs/howtoguides/enable_anbox.md
Outdated
|
||
```{important} | ||
The Anbox Cloud service can only be installed on containers using the --access-only flag. | ||
This option will only setup the APT sources for anbox, but not install any of the snaps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option will only setup the APT sources for anbox, but not install any of the snaps. | |
This option will only set up the APT sources for Anbox, but not install any of the snaps. |
docs/howtoguides/enable_anbox.md
Outdated
``` | ||
|
||
You can also confirm that the service is enabled by running the `pro status` command. | ||
It should contain the following line for `anbox-cloud` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should contain the following line for `anbox-cloud` | |
It should contain the following line for `anbox-cloud`: |
Add a how to guide on how to enable it and add the service to the compatibility matrix
Change the header text on the compatibility matrix to a vertical representation
Fix the compatibility matrix to show that fips-updates and livepatch are compatible
edb0fe7
to
518cc19
Compare
@s-makin updated |
Why is this needed?
We are adding information about the Anbox Cloud service that will be supported through the Pro Client in the next release