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

Notes from working through the tutorial #441

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dafydd-t
Copy link

I've worked through the tutorial with very little context on the project, these are some amendments and comments i have as feedback

Comment on lines +104 to +106
> [!NOTE]
> If you do not see the changes in the plugin UI, try clearing your browser cache.

Copy link
Author

Choose a reason for hiding this comment

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

I spent 20 minutes banging my head against this 😅

@@ -7,18 +7,36 @@ By default, the operator is run as a separate container alongside your grafana d
The operator is a logical pattern which runs one or more controllers. The typical use-case for a controller is the `operator.InformerController`, which holds:
* One or more informers, which subscribe to events for a particular resource kind and namespace
* One or more watchers, which consume events for particular kinds
In our case, the boilerplate code uses the `simple.Operator` operator type, which handles dealing with tying together informers and watchers (or reconcilers) in an `InformerController` for us in `cmd/operator/main.go`:

In our case, the boilerplate code uses the opinionated `simple.App` App type, which handles dealing with tying together informers and watchers (or reconcilers) as Managed Kinds for us in `pkg/app/app.go`:
Copy link
Author

Choose a reason for hiding this comment

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

This bit has not been updated since the refactor away from directly using simple.App. It could do with a redraft by someone more familiar IMO

return nil, fmt.Errorf("unable to create IssueWatcher: %w", err)
}

prometheus.MustRegister(issueWatcher.PrometheusCollectors()...)
Copy link
Author

Choose a reason for hiding this comment

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

This is a bodge that worked for me, i suspect there's a better way to register this collector, but I couldn't see anything useful in simple.App

```go
issueWatcher, err := watchers.NewIssueWatcher()
```
to this:
```go
issueWatcher, err := watchers.NewIssueWatcher(runner.ClientGenerator())
issueWatcher, err := watchers.NewIssueWatcher(k8s.NewClientRegistry(cfg.KubeConfig, k8s.ClientConfig{}))
Copy link
Author

Choose a reason for hiding this comment

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

Another bodge from me to highlight a change required. This worked, but I imagine its not the way it "should" work after the refactor.

@@ -52,7 +52,7 @@ type KindValidator interface {
```
Basically, it consumes an admission request and produces a validation error if validation fails, or returns nil on success.
The `simple` package has a ready-to-go implementation for this: `simple.Validator`, which calls `ValidateFunc` when `Validate` is called.
That's what we're using here, but you can always define your own type to implement `KindValidator`, too.
We use the `simple.Validator` struct here to validate, but you can use any type that implements `simple.KindValidator`.
Copy link
Author

Choose a reason for hiding this comment

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

I couldn't get these changes to apply for me, even after a teardown & clean. But its near the end of the day, I can try again tomorrow!

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.

1 participant