chore: use the juju client for model related methods #1633

kian99 · 2025-06-26T15:19:55Z

Description

This change started off as a simple one, a continuation of #1611 where we slowly move our jujuclient package to use Juju's Go client so that we don't need to call facades directly and we get nicer types out of the client.
The downside of this change is that I had to replace all usages of jujuparams.* with base.* where were were using the RPC object in many places (i.e. an object with JSON tags used for marshalling/unmarshalling over the wire). We are now using a different but similar type that simplifies a few rough edges.

There was also a somewhat unrelated but necessary change in the internal/rpc package where I had previously introduced a dependency on internal/jimm/ that was incorrect and caused cyclic dependencies during this PR. That's now been addressed.

Finally, you'll see new functions in internal/jujuapi/types.go that convert from the base.* types to the API type. I haven't written any tests for these as our integration tests cover this behavior, but as discussed before, if we have any objections to this approach we can always revisit it. Also, confusingly, the types in Juju's base package are not the same types used by Juju's apiserver, this might be worth looking at further.

Fixes JUJU-8186

Engineering checklist

Documentation updated
Covered by unit tests
Covered by integration tests

kian99 · 2025-06-26T15:32:45Z

internal/jujuapi/mapper.go

This file was renamed and modified, see types.go for what existed previously.

alesstimec

1st pass review with a few comments

alesstimec · 2025-07-08T14:32:18Z

internal/errors/errors.go

 	// API Error types.
-	if ec, ok := e.Err.(interface{ ErrorCode() string }); ok {
-		e.Code = Code(ec.ErrorCode())
+	var errCode interface{ ErrorCode() string }


why this change?

The error that comes out of the Juju client is wrapped internally due to a errors.Trace(err) call where that original err has an error code. Then, when do errors.E(err) and try to extract an error code, we get nothing because the error with the code is 2 levels deep. So we can try to unwrap the error until we find an error that implement ErrorCode() or we reach the end of the list of wrapped errors.

alesstimec · 2025-07-08T14:35:36Z

internal/jimm/juju/interface.go


 	// ChangeModelCredential replaces cloud credential for a given model with the provided one.
-	ChangeModelCredential(context.Context, names.ModelTag, names.CloudCredentialTag) error
+	ChangeModelCredential(model names.ModelTag, credential names.CloudCredentialTag) error


i'm not fond of named arguments in interface definitions.. feels like the godoc above does not fully explain what the method is expected to do

Will remove the named arguments and update the godoc.

Removed the named arguments where unneccessary (some were useful and I've left them).
Left the docstring since I didn't touch that in this PR.

internal/jimm/juju/modelbuilder.go

alesstimec · 2025-07-08T14:39:16Z

internal/jujuapi/mapper.go

+	"gopkg.in/yaml.v3"
+)
+
+func cloudFromParams(cloudName string, p jujuparams.Cloud) cloud.Cloud {


it would be nice to have some consistency in naming.. XYToZW where X is a package, Y a type, Z a package and W a type.. e.g. paramsCloudToCloudCloud wdyt?

or have a type that embeds cloud.Cloud and has a FromParamsCloud method

I hear you, I'll take a look at this

Here are some thoughts, we either need to convert from an RPC struct to some type that we use internally or we need to convert to whatever type we use internally to an RPC type - speaking specifically about the jujuapi package.

For converting InternalType -> RPC, we can look at internal/jujuapi/cloud.go where we've got the API handler CloudInfo() which receives a dbmodel.Cloud and runs cloud.ToJujuCloudInfo() on the dbmodel object converting from the dbmodel type to the RPC type. I don't think that is great because it imports the RPC types into the dbmodel package for the purposes of putting the conversion logic in there.

Then for RPC -> InternalType we can look at AddCloud in the same file, which calls cloudFromParams to convert a jujuparams.Cloud to github.com/juju/juju/cloud.Cloud.

So ultimately, my idea is that it would be nice if the naming convention for the mapping functions easily indicated we are pushing data down vs surfacing it.
I.e. if we are converting to an internal type we could name the function toX where X is some type. And if we are converting to an RPC type we could name the function fromX.
The actual words don't matter to me but at least names that reflect which way the data is going. Another options might be rpcToX and xFromRPC.

exactly.. some naming convention where i can tell which way we're converting just by looking at the method name

alesstimec · 2025-07-08T14:41:00Z

internal/jujuclient/dial.go

 	return d.createLoginRequest1(ctx, ctl.ResourceTag(), user.ResourceTag(), p)
 }

+// toModelAccessString maps relation to a model access string.


hhmmm.. i vaguely remember we have a place for these conversion methods between openfga relations to juju access strings, don't we?

Yeah inside internal/jimm/permissions , and you'll see that's where it was used from initially, but I don't think the jujuclient should be reaching in there for things so I've copied it here. Especially because internal/jimm/juju imports this package, so you can get into a dependency cycle very easily (can't remember if this import did cause a cycle but better to avoid importing anything internal/jimm from internal/jujuclient).

The issue makes me think the conversion of OpenFGA types to Juju strings should happen in some kind of adapter package that like internal/openfgaToJuju or at least something along those lines is worth thinking about.

internal/jujuclient/modelmanager.go

alesstimec · 2025-07-08T14:43:40Z

internal/jujuclient/types.go

+// types preferably of the form base.ModelInfo. Until either that inconsitency
+// is fixed or we export the convertParamsModelInfo function, we copy it here
+// here have better consistency in our domain layer.
+func convertParamsModelInfo(modelInfo params.ModelInfo) (base.ModelInfo, error) {


again a comment about naming consistency, or having a type that wraps base.ModelInfo and has a method for conversion from params.ModelInfo and a method that returns the wrapped base.ModelInfo

This function and name was copied from Juju so maybe this one should stay the same?

internal/rpc/httpproxy.go

luci1900 · 2025-08-13T12:33:21Z

So this is a change to use the existing machinery that juju clients rely on for resolving RPC methods to specific ones in case of multiple versions or similar?

Looks reasonable overall, but I don't know if I understand it enough to approve.

luci1900 · 2025-08-13T13:55:35Z

internal/jimm/jimm.go

 	return j.bootstrapManager
 }
+
+// DialerAdapter is an adapter that implements the juju.Dialer interface


Doesn't jujuclient.Dialer also implement Dial in the same way?

Or is this to avoid the pointer?

I'll need to go back and look at this one again.

internal/jimm/juju/interface.go

internal/jimm/juju/modelbuilder.go

luci1900 · 2025-08-13T14:18:38Z

internal/dbmodel/model_test.go

 			Status:      "running",
 			Message:     "ACTIVE",
 		}},
-		SLA: &jujuparams.ModelSLAInfo{


Is this no longer necessary? Or is the zero value actually useful for tests?

kian99 force-pushed the replace-model-manager-client branch from 643ceb3 to fca0e39 Compare June 27, 2025 07:39

kian99 commented Jun 27, 2025

View reviewed changes

internal/jujuapi/mapper.go

Copy link

Contributor Author

kian99 Jun 26, 2025

Choose a reason for hiding this comment

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

This file was renamed and modified, see types.go for what existed previously.

kian99 force-pushed the replace-model-manager-client branch 2 times, most recently from b3b7330 to f7aa97d Compare July 8, 2025 07:51

kian99 marked this pull request as ready for review July 8, 2025 09:13

kian99 requested a review from a team as a code owner July 8, 2025 09:13

alesstimec reviewed Jul 8, 2025

View reviewed changes

kian99 added 5 commits July 21, 2025 08:55

chore: use the juju client for model related methods

982d667

chore: fix broken tests

d9f979b

chore: additional test fixes

81a534c

chore: further test fixes

56fda17

Delete jujuclient/modelmanager_test.go

bf48692

kian99 force-pushed the replace-model-manager-client branch 2 times, most recently from 93eeabf to ec13770 Compare July 21, 2025 07:02

chore: extra docstrings and minor type changes

82493fa

kian99 force-pushed the replace-model-manager-client branch from ec13770 to 82493fa Compare July 23, 2025 09:16

luci1900 reviewed Aug 13, 2025

View reviewed changes

internal/jimm/juju/interface.go Show resolved Hide resolved

luci1900 reviewed Aug 13, 2025

View reviewed changes

internal/jimm/juju/modelbuilder.go Show resolved Hide resolved

luci1900 reviewed Aug 13, 2025

View reviewed changes

chore: use the juju client for model related methods #1633

Are you sure you want to change the base?

chore: use the juju client for model related methods #1633

Uh oh!

Conversation

kian99 commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Engineering checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alesstimec left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kian99 Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kian99 Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

luci1900 commented Aug 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kian99 commented Jun 26, 2025 •

edited

Loading

kian99 Jul 11, 2025 •

edited

Loading

kian99 Jul 11, 2025 •

edited

Loading