-
Notifications
You must be signed in to change notification settings - Fork 188
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
Improve our documentation for Dev Setup #3041
Conversation
|
||
After you've cloned the ASO repo, verify you have access to tags and ensure you have cloned the submodule. | ||
Simply cloning the ASO repo is not enough to successfully run a build. There are two additional things you must ensure 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.
"There are two additional things you must ensure that:"
is a weird end of sentence/setup for the list.
You must also check the following two things:
You must also ensure the following things:
... or some other variant?
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.
Reworded.
|
||
### Verify tags | ||
* You have access to git tags |
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.
* You have access to git tags | |
* You have access to git tags. |
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.
Fixed.
|
||
### Verify tags | ||
* You have access to git tags | ||
Our build scripts depend on tags in order to create a version number |
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.
Our build scripts depend on tags in order to create a version number | |
The build scripts depend on tags in order to create a version number. |
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.
Fixed.
* You have access to git tags | ||
Our build scripts depend on tags in order to create a version number | ||
* The `azure-rest-api-specs` submodule has been cloned | ||
Our code generator parses these specs to determine the shape of our code generated resources. |
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.
Our code generator parses these specs to determine the shape of our code generated resources. | |
The code generator parses these specs to determine the shape of our code generated resources. |
Although actually I almost think you could cut this whole sentence as you say it below too in the more detailed section.
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 want to maintain the parallel structure of having the bullet item followed by an explanatory sentence. That said, I've rephrased it to avoid duplication.
0. Make sure you have installed [the prerequisites to use Docker](https://code.visualstudio.com/docs/remote/containers#_system-requirements), including [WSL](https://learn.microsoft.com/en-us/windows/wsl/install) if on Windows. | ||
1. Install VS Code and the [Remote Development](https://marketplace.visualstudio.com/items?itemName=ms-vscode-remote.vscode-remote-extensionpack) extension (check installation instructions there). | ||
2. Open VS Code | ||
3. Run the VS Code command (with `Ctrl-Shift-P`): `Remote Containers: Clone Repository in Container Volume...` |
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 think that this is now DevContainers: Clone Repository in Container Volume... -- we should update 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.
Updated.
@@ -11,56 +11,6 @@ cascade: | |||
description: "How to contribute new resources to Azure Service Operator v2" | |||
--- | |||
|
|||
## Related pages |
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 feel like we should somehow be linking or highlighting the developer setup page here at least? It's IMO the most important page for people to see. More important even than Directory structure of the operator (which is now the top thing here on the index).
Wondering if we should split the contributing section up entirely and move the other bits to subpages as well and just have index be a TOC-esque thing that links to the various sub-pages?
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've restored the Related Pages
section, for now.
It feels out of place to me, especially given we already have a list of related pages in the lefthand sidebar, plus a table of content in the righthand sidebar. That said, I think we need to restructure this area and I'm happy to leave the section in until we do 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.
LGTM
|
||
Development of ASO on MacOS is possible (one of our team does so), but things are less automated. | ||
|
||
You'll need to manually install the tools as listed by `.devcontainer/install-dependencies.sh`. |
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.
If we don't have it already - Should we have a list of tools/dependencies somewhere?
Just makes it easier for the users, so that they don't have to dig through the .sh
files
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.
Good idea. I'll log a separate task to track doing this.
What this PR does / why we need it:
The documentation lacked clarity and had some errors.
For example:
The performance issues occur if you mount a Windows folder as a volume in the devcontainer, as the folder gets exposed as a network share and things are slow.
If you instead mount a Linux folder from inside WSL as a volume in the devcontainer, performance is just fine.
The above note conflates the two scenarios and is misleading.
Our docs also had some information that wasn't consistently repeated across potential environments.
Special notes for your reviewer:
I've broken the Dev Setup stuff out into a separate page.
How does this PR make you feel:
If applicable: