-
Notifications
You must be signed in to change notification settings - Fork 7
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
Css 4116/backport crossmodel queries #964
Css 4116/backport crossmodel queries #964
Conversation
|
||
// We use very specific formatting parameters to ensure like-for-like output | ||
// with the default juju client installation performing a "status --format json". | ||
formatter := status.NewStatusFormatter(*params) |
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.
Out of curiosity, why does the NewStatusFormatter constructor require a params object that is itself derived from a model? I would've thought a formatter could be declared once outside this loop and then a call to formatter.Format(modelUUID)
would be a more sensible API.
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.
I think they wanted to encapsulate it, but honestly not sure, I would've preferred just a function too honestly
} | ||
// We could use output.NewFormatter() from 3.0+ juju/juju, but ultimately | ||
// we just want some JSON output, regardless of user formatting. As such json.Marshal | ||
// *should* be OK. But TODO: make sure this is fine. |
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.
How will we make sure this is fine. This feels like one of those TODOs that will live forever.
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.
You're right, it probably will lol. But it at least leaves some context... I guess... :D
// | ||
// First, call LoadModel, this will retrieve a model from JIMM's database. | ||
// Next, simply call GetParams. | ||
type formatterParamsRetriever struct { |
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 struct and the whole API around it feel a little to easy to use incorrectly. I see what you mean that it's a sort of builder. It's all internal so not really an issue so feel free to ignore this but what do you think about changing the constructor so that it also accepts a model UUID, and does the loadModel
and dialModel
calls from within the constructor, feel's a little more... cohesive?
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.
Also because currently you can create a formatterParamsRetriever
and if you're using the low level primitives loadModel
and dialModel
and don't do things in the right order, you might forget to run loadModel
, then your dialModel
looks like its working but is dialling the wrong model.
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.
Happy to come and refactor afterwards but yeah, you could misuse it. This is the current state in feature branch, happy to come back and change though afterwards.
"github.com/juju/juju/core/crossmodel" | ||
jujuparams "github.com/juju/juju/rpc/params" |
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.
interesting.. why was this reordered?
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.
Not sure tbh, I directly c/p'd this
Errors: make(map[string][]string), | ||
} | ||
|
||
query, err := gojq.Parse(jqQuery) |
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.
feels query parsing should be part of the jujuapi package.. i think of it like endpoint handler code vs backend code.. this sort of input validation should happen in the endpoint handler code (jujuapi in this case) and should return a bad request error
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.
I think jujuapi is really an entry point, and jimm is a service level package (at least that's what it appears to be setup like). Such that jujuapi handlers call jimm services? Happy to chat on stdup though?
internal/jimm/model_status_parser.go
Outdated
return results, errors.E(op, err) | ||
} | ||
|
||
// We remove "model:" from the UUIDs, unfortunately that's what OpenFGA returns now after |
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.
wait.. what? what model:... uuids should be pure UUIDs not openfga model tags..
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.
In feature-rebac they changed it, it now returns key:value, idk why, but they did in 1.*
internal/jimm/model_status_parser.go
Outdated
// If a result is erroneous, for example, bad data type parsing, the resulting struct field | ||
// Errors will contain a map from model UUID -> []error. Otherwise, the Results field | ||
// will contain model UUID -> []Jq result. | ||
func (j *JIMM) QueryModelsJq(ctx context.Context, modelUUIDs []string, jqQuery string) (params.CrossModelQueryResponse, error) { |
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 seems a bit inefficient. first we can GetUserModels, which returns all models user has access to, then we extract UUIDs.. only to pass them to a method that fetches those same models from the DB again.. let's just pass models []dbmodel.Model as parameter to this method
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.
Ahhh nice spot
|
||
// Set up a formatterParamsRetriever to handle the heavy lifting | ||
// of each facade call and type conversion. | ||
retriever := newFormatterParamsRetriever(j) |
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.
the name is misleading.. as it fetches and formats the model status
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.
It doesn't format the statuses? The formatter.Format() does?
internal/jimm/model_status_parser.go
Outdated
// GetParams retrieves the required parameters for the Juju status formatter from the currently | ||
// loaded model. See formatterParamsRetriever.LoadModel for more information. | ||
func (f *formatterParamsRetriever) GetParams(ctx context.Context, modelUUID string) (*status.NewStatusFormatterParams, error) { | ||
if err := f.loadModel(ctx, modelUUID); err != 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.
this is what i was talking about.. GetUserModels already loads all models.. and here we are fetching it again from the db
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.
Ya refactored now just testing
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.
Seems like this is a pretty important improvement to bring into feature-rebac
as well
} | ||
|
||
// storageListAPI acts as a wrapper over our implementation of the juju client, seen in ./internal/jujuclient. | ||
// This enables us to use storage.GetCombinedStorageInfo without having to c/p the logic we require. |
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.
which bits of logic are we copying?
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.
Without this, we'd need to c/p a lot from juju, this was a few months ago but I remember discussing this with you
@@ -52,6 +54,7 @@ func init() { | |||
r.AddMethod("JIMM", 3, "UpdateMigratedModel", updateMigratedModelMethod) | |||
r.AddMethod("JIMM", 3, "AddCloudToController", addCloudToControllerMethod) | |||
r.AddMethod("JIMM", 3, "RemoveCloudFromController", removeCloudFromControllerMethod) | |||
r.AddMethod("JIMM", 3, "CrossModelQuery", crossModelQueryMethod) |
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.
would we want this to be JIMM 4? i know we only have the two JIMMs in stg and production, but if i were to use the new jimmctl against those, they both have JIMM facade version 3, but not the new method
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.
I'm not sure, happy to do what you think is best
….com/ale8k/jimm into css-4116/backport-crossmodel-queries
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 but i really don't like the formatter naming..
i honestly think that even
thingThatFetchesAndFormatsModelStatus
would be a better name :)
internal/jimm/model_status_parser.go
Outdated
|
||
query, err := gojq.Parse(jqQuery) | ||
if err != nil { | ||
return results, errors.E(op, err) |
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.
errors.E(op, "failed to parse jq query", err)
….com/ale8k/jimm into css-4116/backport-crossmodel-queries
Description
Backports crossmodel queries to v1.
Engineering checklist
Check only items that apply
Test instructions
Notes for code reviewers