-
Notifications
You must be signed in to change notification settings - Fork 141
Document MRD/MRAP/SafeStart #958
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
Conversation
✅ Deploy Preview for crossplane ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
6106ed2
to
7ed06f6
Compare
dc59c59
to
b1f70b3
Compare
Add more resources using wildcard patterns. Update your activation policy: | ||
|
||
```shell | ||
kubectl patch mrap aws-demo-resources --type merge -p '{ |
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.
nit: I prefer kubectl apply -f - <<EOF
with full manifests, so that people can copy paste from any point in the guide, rather than incremental patches. Definitely not a showstopper though.
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 is a continuation of the above. So in general I agree but in this case I am not sure
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 believe the other getting started guides don't currently use the EOF style. This guide should match them closely stylistically.
FWIW preferences like this in our docs have been a big challenge over the years. We've gone back and forth on here docs. Last time we talked about it I believe the consensus was to do what the Kubernetes docs do, which is:
- Example manifests are distinct files in the docs repo
- Our docs site tool (currently Hugo) embeds those files inline
- We include
kubectl apply -f https://docs.crossplane.io/...
links to quickly apply them
IMO the Kube docs approach gives the best of both worlds.
- You can quickly apply a manifest by copying a CLI command
- The example manifest shown inline is plain old YAML, not YAML with arcane shell stuff around 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 don't think we had a tracking issue for this - raised #960.
… MRD/MRAP/SafeStart Signed-off-by: Scott Nichols <[email protected]>
Signed-off-by: Scott Nichols <[email protected]>
Signed-off-by: Scott Nichols <[email protected]>
Signed-off-by: Scott Nichols <[email protected]>
…t page docs Signed-off-by: Scott Nichols <[email protected]>
Signed-off-by: Scott Nichols <[email protected]>
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 just read the getting started guide so far.
I see right as I'm hitting comment that you just moved it though. Some of my comments about sections being too detailed are less important in that context.
{{< hint "tip" >}} | ||
This guide demonstrates the performance and discovery benefits of MRDs by | ||
working with a subset of AWS resources rather than installing hundreds of CRDs. | ||
{{< /hint >}} |
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 imagine a lot of novice/first time users won't know what these issues are, if they've never actually tried to install a billion CRDs. Not sure what the best way to address that is - we probably don't want to spent a ton of time educating them on the problem in the guide.
Before installing providers, it's important to understand Crossplane's default | ||
activation behavior. Crossplane creates a default ManagedResourceActivationPolicy | ||
that, by default, activates **all** managed resources with a `"*"` pattern. |
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.
Before installing providers, it's important to understand Crossplane's default | |
activation behavior. Crossplane creates a default ManagedResourceActivationPolicy | |
that, by default, activates **all** managed resources with a `"*"` pattern. | |
Before installing providers, it's important to understand Crossplane's default | |
activation behavior. Crossplane creates a default `ManagedResourceActivationPolicy` | |
that, by default, activates **all** managed resources with a `"*"` pattern. |
Existing guides use Codeblocks
for all kind names. I'm on the fence about this practice and our docs overall aren't super consistent, but we should match the guides for now.
metadata: | ||
name: provider-aws | ||
spec: | ||
package: xpkg.upbound.io/crossplane-contrib/provider-aws:v0.45.0 |
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 must use xpkg.crossplane.io in these guides. @jeanduplessis mentioned the first build of provider-aws with real v2 support will be v2.0.0.
|
||
{{< hint "important" >}} | ||
The default `"*"` activation pattern defeats the performance benefits of | ||
safe-start by activating all resources. For this tutorial, the guide works with the |
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 is the first mention of safe-start. Probably worth explaining what that is and how it relates to MRDs and MRAPs.
Look for the `connectionDetails` section: | ||
|
||
```yaml | ||
spec: | ||
connectionDetails: | ||
- description: The public IP address assigned to the instance | ||
name: public_ip | ||
type: string | ||
- description: The private IP address assigned to the instance | ||
name: private_ip | ||
type: string | ||
- description: The public DNS name assigned to the instance | ||
name: public_dns | ||
type: string | ||
``` |
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 any providers implement this yet in practice?
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.
no
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'd suggest dropping it then (no-one will be able to 'follow' the guide yet).
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.
how are the providers going to implement them if they don't get documented?
instances.ec2.aws.crossplane.io 2024-01-15T10:30:00Z | ||
``` | ||
|
||
## Create a managed resource |
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.
Could link to the existing MR guide for this instead. I'm picturing a tip "follow the get started with managed resources guide to verify you can create an MR".
# ... other activated MRDs | ||
``` | ||
|
||
## Performance comparison |
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.
Perf, multiple policies, and prod recommendations feel out of place for a getting started guide. I'd move them to concepts.
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.
More review - now through the safe start guide.
|
||
Test safe-start behavior in a real cluster: | ||
|
||
```shell |
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 is a fairly large script to include inline. I wonder if it'd be useful to actually save it somewhere (e.g. outside of docs) for folks to run. Maybe the provider-template repo?
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.
MRAP review
~50% of this feels low value to me. I'd cut most of the examples of different ways to use policies in the wild (team based, env based, use gitops etc etc).
@negz why don't you just edit and push the changes you want? or fork the PR |
Signed-off-by: Scott Nichols <[email protected]>
Signed-off-by: Scott Nichols <[email protected]>
Sure lemme take a shot and see what I come up with. |
first draft of MRD documentation based on the spec and PRs related to MRD/MRAP/SafeStart