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

chore(cleanup): refactoring down to single accounts across all services #935

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jasonmcintosh
Copy link
Member

Significant refactor of accounts to reduce duplication. Prepares for SQL account repository handling.

@jasonmcintosh jasonmcintosh marked this pull request as ready for review February 28, 2023 22:03
return getBackendUpdater().getBackendDatabase().getLocations();
}

@JsonIgnore private transient BackendUpdater backendUpdater;
Copy link

Choose a reason for hiding this comment

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

I know it might be a little early to ask, but if you intend on making these credentials storable, then it would also make sense for them to work similarly through the API. Simply supporting JPA isn't enough; we'd need a way to marshal and unmarshal these through the REST API so that they can be specified from somewhere other than a config file in the first place.

private String apiKey;
private String applicationKey;

@NotNull private RemoteService endpoint;
@JsonIgnore private transient InfluxDbRemoteService influxDbRemoteService;
Copy link

Choose a reason for hiding this comment

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

Might make more sense to use @JacksonInject here instead?

@jvz
Copy link

jvz commented Mar 2, 2023

Looks interesting, but this diverges so much from anything remotely similar to the Kork credentials API that I'm still unsure how this will eventually integrate with secrets, storage, and the REST API. I see a note about some code being potentially useful as a CredentialsParser, but that is an API for turning a CredentialsDefinition into a Credentials object, and this PR seems to combine those two concepts into the one AccountCredentials class. Given the inability to update third party classes to implement Credentials, it seems as though there may be a gap here?

@jasonmcintosh
Copy link
Member Author

SO the goal with this was : Get down to the minimum to work. Remove the cruft. THEN start rebuilding it to match an appropriate API whatever that might look like without breaking/changing any functionality. E.g. This is a pure "cleanup".

Next add features LIKE matching/implementing a common API. There's a potential that depending on how we did things, we might find that we get what we need without a lot of extra work. That said I expect long term to have to bring back at least some of the concepts I'm removing though hopefully in a cleaner way! But it's a lot easier to add afterwards from a clean start than try to clean up as part of a migration.

@jvz
Copy link

jvz commented Mar 16, 2023

I see. We've diverged quite a bit then at this point, so my previous RFC to add RBAC to Kayenta won't be possible until we've figured that out.

@dbyron-sf
Copy link
Contributor

Any chance this can wait til after 1.30? 🤞 that happens "real soon now" ™️

@jasonmcintosh
Copy link
Member Author

Not a rush on my side... see if others pushing for it, and there are other PRs coming I'd think as well on the above. I always considered this a starting refactor.

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