-
Notifications
You must be signed in to change notification settings - Fork 80
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
Add developer docs to README #64
Conversation
6d6d4c6
to
b089f4c
Compare
d73bf60
to
dc6ef8d
Compare
cced855
to
d175cde
Compare
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 lot of good info! I love the helpful examples. One option would be to put this in a developer.MD file instead of the base README, but I think either is fine.
README.md
Outdated
The default schema additionally wraps this proxy store in | ||
[`metrics.Store`](https://pkg.go.dev/github.com/rancher/steve/pkg/stores/metrics#Store), | ||
which records request metrics to Prometheus, by calling | ||
[`metrics.NewNetricsStore`](https://pkg.go.dev/github.com/rancher/steve/pkg/stores/metrics#NewMetricsStore) |
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.
[`metrics.NewNetricsStore`](https://pkg.go.dev/github.com/rancher/steve/pkg/stores/metrics#NewMetricsStore) | |
[`metrics.NewMetricsStore`](https://pkg.go.dev/github.com/rancher/steve/pkg/stores/metrics#NewMetricsStore) |
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.
👍
README.md
Outdated
return types.APIObject{ | ||
Object: map[string]string{ | ||
"value": "[redacted]", | ||
}, | ||
}, nil |
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 think In this case we'd still include the name.
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.
👍
README.md
Outdated
As another example, if you wanted to add custom field to all objects in a | ||
collection response, you can add a schema template with a collection formatter | ||
omit the ID or the group and kind: |
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.
As another example, if you wanted to add custom field to all objects in a | |
collection response, you can add a schema template with a collection formatter | |
omit the ID or the group and kind: | |
As another example, if you wanted to add custom field to all objects in a | |
collection response, you can add a schema template with a collection formatter | |
to omit the ID or the group and kind: |
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.
👍
Rancher implements aggregation for other types of services as well. In Rancher, | ||
the user can define endpoints via a | ||
[v3.APIService](https://pkg.go.dev/github.com/rancher/rancher/pkg/apis/management.cattle.io/v3#APIService) | ||
custom resource (which is distinct from the built-in Kubernetes | ||
[v1.APIService](https://kubernetes.io/docs/reference/kubernetes-api/cluster-resources/api-service-v1/) | ||
resource. Then Rancher runs a middleware handler that routes incoming requests | ||
to defined endpoints. The external services follow the same process of using a | ||
defined secret containing a URL and token to connect and authenticate to | ||
Rancher. This aggregation is defined independently and does not use steve's | ||
aggregation client. |
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: feels like this portion could just be in the rancher README.md.
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 could add it there as well, but I and others have been confused about "steve aggregation" and what it really means, since there is a significant amount of code in steve dedicated to this not-steve functionality, so I prefer to keep it here.
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
rancher/rancher#38593