More cleanup #11
More cleanup #11
Conversation
After this set of changes, and some modifications to the `go.mod` the tests and code now compiles and runs. The things I've removed I don't believe are needed for this layer but feel free to comment on them. My next step is to setup the CI so that even if failing the tests run properly and start a more aggressive refactor from there.
|
Please pay attention to the removals - I think they don't belong on this layer and will stay within Grafana, so it's important to stop and think, "does this make sense to live in Grafana?" with the code I've removed. |
gotjosh
requested review from
alexweav,
grobinson-grafana,
yuri-tceretian and
joeblubaugh
| @@ -0,0 +1 @@ | |||
| package alerting | |||
There was a problem hiding this comment.
Having two grafana_alertmanager and mimir_alertmanager files in the shared utils feels strange to me, since both grafana and the extracted mimir AM will eventually depend on this package. Shouldn't any implementation specific code live in the implementation?
There was a problem hiding this comment.
No on this stage - the design proposed that we first unify without modification so that we can import from both Grafana and Mimir as soon as possible.
Trying to unify before importing seems like an unnecessary risk, given, that we can unify as much as we want of both structures while still depending on two separate structures.
|
|
||
| return orgAM, nil | ||
| } | ||
|
|
||
| // NilPeer and NilChannel implements the Alertmanager clustering interface. | ||
| type NilPeer struct{} |
There was a problem hiding this comment.
Is this type useful anymore? Does it make any sense to keep this in multiorg_alertmanager.go as the only object, should the file be renamed?
There was a problem hiding this comment.
This type is def useful on this layer - NilPeer represents a null object for the clustering interface of the Alertmanager. However, I think you're right that this might not be its home - I've made a note in #8
| type Crypto interface { | ||
| LoadSecureSettings(ctx context.Context, orgId int64, receivers []*definitions.PostableApiReceiver) error | ||
| Encrypt(ctx context.Context, payload []byte, opt secrets.EncryptionOptions) ([]byte, error) | ||
|
|
||
| getDecryptedSecret(r *definitions.PostableGrafanaReceiver, key string) (string, error) | ||
| } |
There was a problem hiding this comment.
Grafana and mimir are almost certainly going to have different crypto implementations.
But, perhaps it'd be nice to keep a simpler shared interface for crypto defined here? Each project could then have an adapter that links its own crypto implementation to this interface.
That way, we don't have to really work around crypto at this level. It's an external dependency that is used in several places. A shared representation of it would be useful.
(If we do keep such an interface, it'd need simpler/different methods. These method names are taken directly from Grafana)
There was a problem hiding this comment.
Good idea, I've made a note in #8
After this set of changes, and some modifications to the
go.modthe tests and code now compiles and runs. The things I've removed I don't believe are needed for this layer but feel free to comment on them.My next step is to setup the CI so that even if failing the tests run properly and start a more aggressive refactor from there.