-
Notifications
You must be signed in to change notification settings - Fork 102
CMR concepts and scenarios #2248
Conversation
evilnick
left a comment
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.
Just some initial comments, I will have more!
src/en/models-cmr-scene-1.md
Outdated
| juju add-model wordpress-model | ||
| juju deploy wordpress | ||
| juju expose wordpress | ||
| juju relate wordpress:db admin/cmr-model.mysql |
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.
we don't use relate, it is an alias
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.
Agreed.
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.
Do we keep juju status everywhere? It's an alias for juju show-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.
What is an alias vs not isn't that obvious to the user and I think that defaulting to the shortest commands is reasonable.
|
|
||
| ```bash | ||
| juju add-relation mediawiki:db mysql | ||
| juju add-relation mysql mediawiki: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.
why is the order reversed? I mean, it shouldn't matter, but we have always used order in the examples
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.
To mirror the previous command: juju add-relation mysql wordpress.
What "order" are you talking about when you say "we have always used order in the examples"?
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.
From the thing doing the giving, to the consumer. wordpress consumes mysql, as does mediawiki
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.
So doesn't my new order follow your policy? You said the giver comes first. I've changed it so that mysql, the giver (doing the giving), comes first.
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.
Yes, sorry it does now
| relation interface identifier. In this case, we want MySQL to provide the | ||
| backend database for mediawiki ('db' relation), so this is what we need to | ||
| enter: | ||
| The solution is to be explicit when referring to an *endpoint*, where the |
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.
If we are going to start calling these objects 'endpoints', I think we should actually introduce them a bit more than that
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.
Right, I made this change minimal because it's not strictly a CMR thing, but it really needed changing. I didn't want to get caught in a rabbit hole. You want more of this here or do you want this done in another PR?
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.
Maybe we should tack this on in a different pr when we have nailed down the CMR content
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.
OK. I've left this text commented.
|
|
||
| This scenario describes a MediaWiki deployment, based within the same (LXD) | ||
| controller; used by the admin user, and consumed by multiple models. | ||
|
|
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.
Maybe explain the cmr aspect of this in some way first. it is a bit confusing to go straight from this statement to creating a controller...
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.
OK, but it is an example scenario, and I do refer to the main page for background information.
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.
After re-reading it looks OK. The very first sentence says the scenario pertains to CMR (link). Do you really want me to re-defined what CMR is?
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.
No, I just think this doesn't start well - it mentions mediawiki briefly and then suddenly you are building a controller (or actually, bizarrely, adding credentials).
I think the whole thing would work a lot better if we actually set out what we were going to do, and lay out some requirements (e.g. In this example we have a controller called xxx and a model called yyy. Nobody cares that it is on aws. Then you can start the actual tutorial with something that matters, like adding mysql.
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.
OK, I thought your comment was for scenario 2. It makes more sense now.
src/en/models-cmr-scene-1.md
Outdated
| ``` | ||
|
|
||
| The output to `juju status` should eventually look similar to: | ||
| !!! Note: |
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 don't think this note is very noteworthy
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.
OK, but I wanted to make clear that a special CMR model is not required.
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 actually sure that is helped by showing people the steps to create a controller and a model. We could just state that we have started with a model called whatever, and leave it to the reader to set up whatever model/controller they want.
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 feel that due to the various contexts (what controller, what model) it is important to be explicit.
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 just think the note is distracting. If it is felt to be important to mention you can use any model, you could just mention that when you create the ones above. It isn't a big deal, it just feels like a big interruption for the reader for something minor(is)
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 removed the Note.
src/en/models-cmr-scene-2.md
Outdated
| ```bash | ||
| juju find-offers admin/default | ||
| juju find-offers ian:admin/default | ||
| juju find-offers lxd-cmr-1:admin/cmr-model-1 |
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.
is it always necessary, when targeting a model, to include the user name too? I mean, it seems to be from my experiments, but is that really true?
I guess what I am saying is that I would assume
juju find-offers lxd-cmr-1:cmr-model-1
would work. It doesn't. Am I missing something?
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 username pertains to the offering side and I don't see how the consuming side can guess what it is.
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.
Sorry, I misunderstood.
So what you say makes sense and I tried it and it works for me. 🐰
juju find-offers lxd-cmr-1:cmr-model-1
Store URL Access Interfaces
lxd-cmr-1 admin/cmr-model-1.mysql admin mysql: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.
That is even more worrying! I will try again tomorrow
src/en/models-cmr-scene-2.md
Outdated
|
|
||
| In this case, the relate --via option is used to inform the offering side so | ||
| that the correct firewall rules can be set up. | ||
| This will work providing the offering side did not previously set up a |
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 find this paragraph confusing. Does this mean that it will work if no whitelist has been set?
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 agree I can make it clearer. The answer to your question is yes, without a whitelist any request for a firewall rule will be accepted.
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 done, and with less words.
src/en/models-cmr.md
Outdated
|
|
||
| [`consume`][commands-consume] | ||
| : Adds a remote offer to a model. | ||
| : Adds a remote offer to a model without relating to it. |
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 the most confusing one by far. IIUC, this is like a reverse offer, as in this case it is the 'consumer' saying, I would be happy for someone to provide me with this interface.
In which case:
-
I don't understand why this can't be done with the offer command anyhow - as far as anyone is concerned the relation is just a pipe and it doesn't matter what you label the ends
-
if we are doing it this way, the description needs to be a lot clearer (the help for this command also needs work)
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.
Okay, having read some more I think I understand this better now. 'consume' is like, take up the offer, but don't attach it to anything, save it for later? How do you attach to it later? How do you know it exists?
The comments about the description still stand (this is a problem with ad-hoc gerunds).
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.
Suggestions welcome 😺
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 about : "Accepts an offer from a different model without immediately adding a relation to it. The offered application will subsequently appear as an application in the current model (e.g. in juju status) and a relation can made as with any other native application."
Or something
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.
That's awfully long!
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.
Adds is a word I had difficulty with here - it could be read as the user is "adding" (creating) the offer rather than consuming it. Also, I don't see why the explanation needs to be one line?
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.
Because all the other commands have short descriptors. I'd like to avoid long lines here. I agree that it comes off as abrupt since these commands are listed from the outset, before the reader has a chance to digest anything. This is the same approach I took with storage. I asked for the team's opinion on that and the consensus was that it was 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.
What about "Links an offer to a 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.
It also doesn't help that the consume command is the very first one (due to the alphabetical order).
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.
What is wrong with "accepts"?
It is fine when the descriptions actually work.
src/en/models-cmr.md
Outdated
| the same controller. | ||
|
|
||
| `juju relate <application> <offer url>` | ||
| `juju relate <application>[:<application endpoint>] <offer url>[:<offer endpoint>]` |
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.
add-relation
I suppose in the sense that offers are external objects, doing things this way around feels correct. I wonder if it would then make sense to change every other add-relation command in the docs
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 how it is more correct by using relate. I would like to stay consistent by not using aliases. Maybe I'm missing your point.
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 meant the order of the application / offer url. Normally we do add-relation <provider> <consumer>
we should definitely use add-relation
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.
Ohh
src/en/models-cmr.md
Outdated
| An offer can be consumed without relating to it. This workflow sets up the | ||
| proxy application in the consuming model and creates a user-defined alias for | ||
| the offer. This latter is what's used to relate to. Having an offer alias can | ||
| avoid a namespsce conflict with a pre-existing application. |
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.
namespace
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.
OK
|
I believe all your concerns have been addressed. |
|
Okay, we can probably merge this and continue to refine it. One thing I think we ought to address is the use (maybe overuse) of CMR as an acronym to define what is going on here. We may call this feature CMR internally, but I'm not sure it makes sense to the user. |
fixes #2225