Skip to content

Commit

Permalink
Refactor according to PR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
ale8k committed Jun 26, 2023
1 parent b536670 commit 25f298e
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 63 deletions.
46 changes: 14 additions & 32 deletions internal/jimm/model_status_parser.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package jimm

import (
"context"
"database/sql"
"encoding/json"

"github.com/CanonicalLtd/jimm/api/params"
Expand All @@ -25,7 +24,7 @@ import (
// 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) {
func (j *JIMM) QueryModelsJq(ctx context.Context, models []dbmodel.Model, jqQuery string) (params.CrossModelQueryResponse, error) {
op := errors.Op("QueryModels")
results := params.CrossModelQueryResponse{
Results: make(map[string][]any),
Expand All @@ -41,12 +40,12 @@ func (j *JIMM) QueryModelsJq(ctx context.Context, modelUUIDs []string, jqQuery s
// of each facade call and type conversion.
retriever := newFormatterParamsRetriever(j)

for _, id := range modelUUIDs {

params, err := retriever.GetParams(ctx, id)
for _, model := range models {
modelUUID := model.UUID.String
params, err := retriever.GetParams(ctx, model)
if err != nil {
zapctx.Error(ctx, "failed to get status formatter params", zap.String("model-uuid", id))
results.Errors[id] = append(results.Errors[id], err.Error())
zapctx.Error(ctx, "failed to get status formatter params", zap.String("model-uuid", modelUUID))
results.Errors[modelUUID] = append(results.Errors[modelUUID], err.Error())
continue
}

Expand All @@ -56,17 +55,17 @@ func (j *JIMM) QueryModelsJq(ctx context.Context, modelUUIDs []string, jqQuery s

formattedStatus, err := formatter.Format()
if err != nil {
zapctx.Error(ctx, "failed to format status", zap.String("model-uuid", id))
results.Errors[id] = append(results.Errors[id], err.Error())
zapctx.Error(ctx, "failed to format status", zap.String("model-uuid", modelUUID))
results.Errors[modelUUID] = append(results.Errors[modelUUID], err.Error())
continue
}
// 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.
fb, err := json.Marshal(formattedStatus)
if err != nil {
zapctx.Error(ctx, "failed to marshal formatted status", zap.String("model-uuid", id))
results.Errors[id] = append(results.Errors[id], err.Error())
zapctx.Error(ctx, "failed to marshal formatted status", zap.String("model-uuid", modelUUID))
results.Errors[modelUUID] = append(results.Errors[modelUUID], err.Error())
continue
}
tempMap := make(map[string]any)
Expand All @@ -85,11 +84,11 @@ func (j *JIMM) QueryModelsJq(ctx context.Context, modelUUIDs []string, jqQuery s
// query. As such, we simply append all to the errors field and continue to collect
// both erreoneous and valid query results.
if err, ok := v.(error); ok {
results.Errors[id] = append(results.Errors[id], "jq error: "+err.Error())
results.Errors[modelUUID] = append(results.Errors[modelUUID], "jq error: "+err.Error())
continue
}

results.Results[id] = append(results.Results[id], v)
results.Results[modelUUID] = append(results.Results[modelUUID], v)
}
}
return results, nil
Expand Down Expand Up @@ -118,10 +117,8 @@ func newFormatterParamsRetriever(j *JIMM) *formatterParamsRetriever {

// 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 {
return nil, err
}
func (f *formatterParamsRetriever) GetParams(ctx context.Context, model dbmodel.Model) (*status.NewStatusFormatterParams, error) {
f.model = &model

err := f.dialModel(ctx)
if err != nil {
Expand All @@ -148,21 +145,6 @@ func (f *formatterParamsRetriever) GetParams(ctx context.Context, modelUUID stri
}, nil
}

// LoadModel loads the model by UUID from the database into the formatterParamsRetriever.
// This MUST be called before attempting to GetParams().
func (f *formatterParamsRetriever) loadModel(ctx context.Context, modelUUID string) error {
model := dbmodel.Model{
UUID: sql.NullString{String: modelUUID, Valid: true},
}

if err := f.jimm.Database.GetModel(ctx, &model); err != nil {
zapctx.Error(ctx, "failed to retrieve model", zap.String("model-uuid", modelUUID))
return err
}
f.model = &model
return nil
}

// dialModel dials the model currently loaded into the formatterParamsRetriever.
func (f *formatterParamsRetriever) dialModel(ctx context.Context) error {
modelTag, ok := f.model.Tag().(names.ModelTag)
Expand Down
32 changes: 9 additions & 23 deletions internal/jimm/model_status_parser_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
jujuparams "github.com/juju/juju/rpc/params"

"github.com/CanonicalLtd/jimm/internal/db"
"github.com/CanonicalLtd/jimm/internal/dbmodel"
"github.com/CanonicalLtd/jimm/internal/errors"
"github.com/CanonicalLtd/jimm/internal/jimm"
"github.com/CanonicalLtd/jimm/internal/jimmtest"
Expand Down Expand Up @@ -440,14 +441,14 @@ func TestQueryModelsJq(t *testing.T) {
}

// Query for all models only.
userModels, err := j.Database.GetUserModels(ctx, &user)
usersModels, err := j.Database.GetUserModels(ctx, &user)
c.Assert(err, qt.IsNil)
userModelUUIDs := make([]string, len(userModels))
for i, m := range userModels {
userModelUUIDs[i] = m.Model_.UUID.String
models := make([]dbmodel.Model, len(usersModels))
for i, m := range usersModels {
models[i] = m.Model_
}

res, err := j.QueryModelsJq(ctx, userModelUUIDs, ".model")
res, err := j.QueryModelsJq(ctx, models, ".model")
c.Assert(err, qt.IsNil)
c.Assert(`
{
Expand Down Expand Up @@ -507,12 +508,7 @@ func TestQueryModelsJq(t *testing.T) {
`, qt.JSONEquals, res)

// Query all applications across all models.
// Reset the model UUID for test (as it mutates the slice)
for i, m := range userModels {
userModelUUIDs[i] = m.Model_.UUID.String
}

res, err = j.QueryModelsJq(ctx, userModelUUIDs, ".applications")
res, err = j.QueryModelsJq(ctx, models, ".applications")
c.Assert(err, qt.IsNil)
c.Assert(`
{
Expand Down Expand Up @@ -660,12 +656,7 @@ func TestQueryModelsJq(t *testing.T) {
`, qt.JSONEquals, res)

// Query specifically for models including the app "nginx-ingress-integrator"
// Reset the model UUID for test (as it mutates the slice)
for i, m := range userModels {
userModelUUIDs[i] = m.Model_.UUID.String
}

res, err = j.QueryModelsJq(ctx, userModelUUIDs, ".applications | with_entries(select(.key==\"nginx-ingress-integrator\"))")
res, err = j.QueryModelsJq(ctx, models, ".applications | with_entries(select(.key==\"nginx-ingress-integrator\"))")
c.Assert(err, qt.IsNil)
c.Assert(`
{
Expand Down Expand Up @@ -727,12 +718,7 @@ func TestQueryModelsJq(t *testing.T) {
`, qt.JSONEquals, res)

// Query specifically for storage on this model.
// Reset the model UUID for test (as it mutates the slice)
for i, m := range userModels {
userModelUUIDs[i] = m.Model_.UUID.String
}

res, err = j.QueryModelsJq(ctx, userModelUUIDs, ".storage")
res, err = j.QueryModelsJq(ctx, models, ".storage")
c.Assert(err, qt.IsNil)

// Not the cleanest thing in the world, but this field needs ignoring,
Expand Down
14 changes: 6 additions & 8 deletions internal/jujuapi/jimm.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -451,19 +451,17 @@ func (r *controllerRoot) RemoveCloudFromController(ctx context.Context, req apip
func (r *controllerRoot) CrossModelQuery(ctx context.Context, req apiparams.CrossModelQueryRequest) (apiparams.CrossModelQueryResponse, error) {
const op = errors.Op("jujuapi.CrossModelQuery")

models, err := r.jimm.Database.GetUserModels(ctx, r.user)
modelUUIDs := make([]string, len(models))

for i, m := range models {
modelUUIDs[i] = m.Model_.UUID.String
}

usersModels, err := r.jimm.Database.GetUserModels(ctx, r.user)
if err != nil {
return apiparams.CrossModelQueryResponse{}, errors.E(op, errors.Code("failed to get models for user"))
}
models := make([]dbmodel.Model, len(usersModels))
for i, m := range usersModels {
models[i] = m.Model_
}
switch strings.TrimSpace(strings.ToLower(req.Type)) {
case "jq":
return r.jimm.QueryModelsJq(ctx, modelUUIDs, req.Query)
return r.jimm.QueryModelsJq(ctx, models, req.Query)
case "jimmsql":
return apiparams.CrossModelQueryResponse{}, errors.E(op, errors.CodeNotImplemented)
default:
Expand Down

0 comments on commit 25f298e

Please sign in to comment.