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

Add to README #220

Merged
merged 1 commit into from
Aug 30, 2022
Merged

Add to README #220

merged 1 commit into from
Aug 30, 2022

Conversation

rmweir
Copy link
Contributor

@rmweir rmweir commented Jul 25, 2022

No description provided.

@rmweir rmweir requested a review from KevinJoiner July 28, 2022 14:17
README.md Outdated
Comment on lines 75 to 100
```golang
controllergen.Run(args.Options{
OutputPackage: "github.com/rancher/rancher/pkg/generated",
Boilerplate: "scripts/boilerplate.go.txt",
Groups: map[string]args.Group{
"management.cattle.io": {
PackageName: "management.cattle.io",
Types: []interface{}{
// All structs with an embedded ObjectMeta field will be picked up
"./pkg/apis/management.cattle.io/v3",
v3.ProjectCatalog{},
v3.ClusterCatalog{},
},
GenerateTypes: true,
},
"ui.cattle.io": {
PackageName: "ui.cattle.io",
Types: []interface{}{
"./pkg/apis/ui.cattle.io/v1",
},
GenerateTypes: true,
},
},
})
```
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be helpful to add comments on member variables in this piece of code so that someone looking at this for the first time understands what each piece is being used for?
If no comments are added here, then a link to the struct in code with comments should be added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean in options or the catalog structs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe both? comments for the args.Options, but now I am curious about where the embedded Catalog structs are defined. Were they generated beforehand somehow? Or can these structs be any struct that has an embedded k8s.io/apimachinery/pkg/apis/meta/v1.ObjectMeta struct?

Suggested change
```golang
controllergen.Run(args.Options{
OutputPackage: "github.com/rancher/rancher/pkg/generated",
Boilerplate: "scripts/boilerplate.go.txt",
Groups: map[string]args.Group{
"management.cattle.io": {
PackageName: "management.cattle.io",
Types: []interface{}{
// All structs with an embedded ObjectMeta field will be picked up
"./pkg/apis/management.cattle.io/v3",
v3.ProjectCatalog{},
v3.ClusterCatalog{},
},
GenerateTypes: true,
},
"ui.cattle.io": {
PackageName: "ui.cattle.io",
Types: []interface{}{
"./pkg/apis/ui.cattle.io/v1",
},
GenerateTypes: true,
},
},
})
```
```golang
controllergen.Run(args.Options{
// Location to store generated packages.
OutputPackage: "github.com/rancher/rancher/pkg/generated",
// Whatever Boilerplate is.
Boilerplate: "scripts/boilerplate.go.txt",
// Things you want to generate. Is the key always the APIGroup.
Groups: map[string]args.Group{
"management.cattle.io": {
// Does this have to match the APIGroup?
PackageName: "management.cattle.io",
// List of string package names or structs? can they be pointers?
Types: []interface{}{
// All structs with an embedded ObjectMeta field will be picked up
"./pkg/apis/management.cattle.io/v3",
v3.ProjectCatalog{},
v3.ClusterCatalog{},
},
// What gets generated if this is false?
GenerateTypes: true,
},
"ui.cattle.io": {
PackageName: "ui.cattle.io",
Types: []interface{}{
"./pkg/apis/ui.cattle.io/v1",
},
GenerateTypes: true,
},
},
})

Copy link
Contributor Author

@rmweir rmweir Aug 16, 2022

Choose a reason for hiding this comment

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

It's the latter. The Catalog is generated above since it is in the named package and directly has the ObjectMeta field. The ClusterCatalog and ProjectCatalog types are explicitly named because they don't directly have the ObjectMeta field and instead embed a struct that does. They are both basically namespaced versions of the Catalog type. I have added explanations for this and provided an additional example for a simpler type.

@KevinJoiner
Copy link
Contributor

Related to #205

README.md Outdated

## Useful Definitions:
### factory:
Controllers are categorized by their API group and bundled into a struct called a factory.
Copy link
Contributor

Choose a reason for hiding this comment

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

The factory is used for generating controllers, right?

Copy link
Contributor Author

@rmweir rmweir Aug 16, 2022

Choose a reason for hiding this comment

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

I have updated this comment multiple times as I've revised the definition to add more detail here as I think the current explanation isn't sufficient. I have updated it to better explain what "factory" is referring to and what they do. Lmk what your thoughts on the change and if it clarifies anything.

README.md Outdated
Watch(namespace string, opts metav1.ListOptions) (watch.Interface, error)
// Patch will create or updates the given object. Patch type specifies the how the data is structured, i.e. json
Copy link
Contributor

Choose a reason for hiding this comment

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

create

is this true?

Copy link
Contributor Author

@rmweir rmweir Aug 16, 2022

Choose a reason for hiding this comment

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

I think this is conditionally true. If the patch type is application/apply-patch+yaml, then yes. The second part about patch type is incorrect though. I've updated to give a more general description of patch, fixed the inaccuracy, and provided some additional documentation links since this is a more complicated method which does not seem well understood by the Kubernetes community in general.

@rmweir rmweir force-pushed the add-docs branch 2 times, most recently from de86f07 to 5cf5062 Compare August 16, 2022 23:14
@rmweir rmweir requested a review from cmurphy August 17, 2022 00:06
Copy link
Contributor

@cmurphy cmurphy left a comment

Choose a reason for hiding this comment

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

lgtm minus a couple typos and the squash

Copy link
Contributor

@KevinJoiner KevinJoiner left a comment

Choose a reason for hiding this comment

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

LGTM - minus one typo

@rmweir rmweir merged commit ecf879c into rancher:master Aug 30, 2022
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.

3 participants