fix(internal/db/cloud.go): add virtual field to CloudRegion
#1516
Conversation
| grantJIMMModelAdmin: func(_ context.Context, _ names.ModelTag) error { | ||
| return nil | ||
| }, | ||
| createModel: assertCreateModelArgs(&jujuparams.ModelCreateArgs{ |
There was a problem hiding this comment.
This test case and the corresponding assertCreateModelArgs are super confusing to read. Why is this pattern needed?
There was a problem hiding this comment.
just continuing the pattern that was here before which is not all that confusing.. think of it as middleware: each function checks a certain aspect of the call and passes check to the next one
There was a problem hiding this comment.
I took a look at the other test cases and I see what you mean that it's following the pattern. I think they're all extremely confusing, the whole test is really hard to read.
In any case, regarding the stuff inside assertCreateModelArgs. Why are we asserting that the fields match when we also have expectModel which is likely being used inside the test to verify the created model matches what is expected.
There was a problem hiding this comment.
expectModel asserts that our database looks the way we expect it - it does not assert the arguments sent to the controller to create a new model.
the assertCreateModelArgs asserts that what we send to the controller contains parameters we thing should be sent to the controller - in this test case the cloud region should not be sent since it's a virtual one
There was a problem hiding this comment.
Ahh I understand, that is nice but tough to parse. Grabbing lunch then I'll take a closer look with that in mind. But seems fine to me
alesstimec
force-pushed
the
virtual-default-cloud-region
branch
from
2c5e0c4 to
797578b
Compare
| // clouds with no regions; e.g. maas or k8s clouds may not have regions, | ||
| // yet for some cloud juju creates a "default" region, while for others | ||
| // it does not. | ||
| Virtual bool |
There was a problem hiding this comment.
premise: i don't know if you can add a region after a cloud has been created.
In case you can, we should mark the region as virtual and then remove the virtual region once a real region is added.
There was a problem hiding this comment.
additional question: if we mark the region as Virtual we don't need to add a fake default region anymore as you did in the previous PR. Right?
There was a problem hiding this comment.
i do not know if it is possible to add regions: theoretically we could add a controller that would bring with it additional regions for a cloud.. but then that becomes a more complicated issue - is this a public cloud, is it private, is it even the same cloud, etc..
on the second questions: yes, we still need to add a default "virtual" region
When we add a cloud reports no cloud regions we add a virtual "default" region to keep the controller lookup logic the same when adding a model. Then only difference is that we do not send the cloud region name in ModelCreateArgs to the controller.
alesstimec
force-pushed
the
virtual-default-cloud-region
branch
from
797578b to
5b8b195
Compare
Description
When we add a cloud reports no cloud regions we add a virtual "default" region to keep the
controller lookup logic the same when adding a model. Then only difference is that we do not send
the cloud region name in ModelCreateArgs to the controller.
Engineering checklist
Test instructions