Issue 5849054: Environment settings explicitly set.
DescriptionEnvironment settings explicitly set.
Change environment settings from a transparent sync to an explict set/get after
initialization.
https://code.launchpad.net/~hazmat/juju/environment-settings/+merge/98055
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 17
Patch Set 2 : Environment settings explicitly set. #Patch Set 3 : Environment settings explicitly set. #
MessagesTotal messages: 10
Please take a look.
Sign in to reply to this message.
Doesn't appear to address changes to provider-access settings, for which I think the original model remains a surprisingly good fit. Otherwise LGTM. https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... File source/drafts/environment-settings.rst (right): https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... source/drafts/environment-settings.rst:18: existing runtime environment, but future usage of these settings. Just a thought: it makes a certain amount of sense to update provider access settings automatically; I can't forsee people changing them without needing to do so. https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... source/drafts/environment-settings.rst:46: bootstrap and will be serialized into the environment via:: So, provider credentials will still be supplied lazily (good); is there actually any reason not to keep this aspect using exactly the original always-update behaviour? (For provider access settings only, ofc.)
Sign in to reply to this message.
Looks great. Details only: https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... File source/drafts/environment-settings.rst (right): https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... source/drafts/environment-settings.rst:12: non bootstrap command. Some left-over here. https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... source/drafts/environment-settings.rst:25: An example of juju-set usage:: s/juju-set/set-env/ https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... source/drafts/environment-settings.rst:27: juju set-env apt-proxy http://us-east-1.ec2.archive.ubuntu.com.s3.amazonaws.com/ubuntu/ Can we have an equals sign within each option. E.g.: juju set-env foo=bar foz=baz It'd also be good to have a word about validation. set-env should blow up with unsupported keys, probably implemented with a schema validator. https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... source/drafts/environment-settings.rst:45: Non provider-credential settings can be specified at when using juju s/at // https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... source/drafts/environment-settings.rst:48: juju bootstrap apt-proxy=http://bit.ly ec2-instance-type=m1.large ec2-region=eu-west-1 I hadn't thought of using raw arguments. It looks nice. https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... source/drafts/environment-settings.rst:52: juju get-env What does that return? JSON? Should it take arguments instead?
Sign in to reply to this message.
Thanks for the reviews guys. https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... File source/drafts/environment-settings.rst (right): https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... source/drafts/environment-settings.rst:18: existing runtime environment, but future usage of these settings. On 2012/03/17 14:49:30, fwereade wrote: > Just a thought: it makes a certain amount of sense to update provider access > settings automatically; I can't forsee people changing them without needing to > do so. Any sync is problematic for multi-user usage. Say we have different accounts, sounds like a mess :-). Explicit set-env for provider creds sounds quite reasonable to me. https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... source/drafts/environment-settings.rst:48: juju bootstrap apt-proxy=http://bit.ly ec2-instance-type=m1.large ec2-region=eu-west-1 On 2012/03/20 19:23:51, niemeyer wrote: > I hadn't thought of using raw arguments. It looks nice. it looks nice, but it might be considered inconsistent with the other usage which is via a --constraints flag. https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... source/drafts/environment-settings.rst:52: juju get-env On 2012/03/20 19:23:51, niemeyer wrote: > What does that return? JSON? Should it take arguments instead? YAML, with a flag for json. I'm realizing that we probably need another flag or require a key to access provider settings. as we get to more multi user envs or delegated usage, allowing everyone to retrieve the provider keys by default seems suspect. Otoh requiring a key always seems heavy handed since its an introspection tool. what do you think about a happy medium that provider credentials require a --provider flag or arg. This also maps well to the proposed storage mode.
Sign in to reply to this message.
https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... File source/drafts/environment-settings.rst (right): https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... source/drafts/environment-settings.rst:48: juju bootstrap apt-proxy=http://bit.ly ec2-instance-type=m1.large ec2-region=eu-west-1 On 2012/03/21 08:19:05, hazmat wrote: > On 2012/03/20 19:23:51, niemeyer wrote: > > I hadn't thought of using raw arguments. It looks nice. > > it looks nice, but it might be considered inconsistent with the other usage > which is via a --constraints flag. IMO separating the arguments into constraints/non-constraints is unhelpfully icky; set-constraints does currently use raw args, but that's just because there's no possibility of ambiguity: they're *all* constraints, rather than being a mix of env settings and env sub-settings (as it were).
Sign in to reply to this message.
Another round. https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... File source/drafts/environment-settings.rst (right): https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... source/drafts/environment-settings.rst:48: juju bootstrap apt-proxy=http://bit.ly ec2-instance-type=m1.large ec2-region=eu-west-1 On 2012/03/21 08:33:26, fwereade wrote: > IMO separating the arguments into constraints/non-constraints is unhelpfully > icky; set-constraints does currently use raw args, but that's just because > there's no possibility of ambiguity: they're *all* constraints, rather than > being a mix of env settings and env sub-settings (as it were). Indeed. I hadn't realized that this was mixing both. I suggest we have the environment settings as real flags for bootstrap. For example, in the above case: juju bootstrap --apt-proxy=http://bit.ly \ --constraints="ec2-instance-type=m1.large" This way it's still homogeneous, but in the other direction. https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... source/drafts/environment-settings.rst:52: juju get-env On 2012/03/21 08:19:05, hazmat wrote: > YAML, with a flag for json. Sounds good. I'd even drop json for now. > I'm realizing that we probably need another flag or require a key to access > provider settings. as we get to more multi user envs or delegated usage, > allowing everyone to retrieve the provider keys by default seems suspect. I think it's fine to show whatever they've got access to. Just not showing when they actually do have access isn't really secure, and when we implement access control we can easily filter out provider credentials if the user doesn't have it. > Otoh requiring a key always seems heavy handed since its an introspection tool. > > what do you think about a happy medium that provider credentials require a > --provider flag or arg. This also maps well to the proposed storage mode. I feel it's unnecessary. Just show everything for now. When we implement access control, hide to those who can't see it.
Sign in to reply to this message.
On 2012/03/21 14:34:13, niemeyer wrote: > Another round. > > https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... > File source/drafts/environment-settings.rst (right): > > https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... > source/drafts/environment-settings.rst:48: juju bootstrap > apt-proxy=http://bit.ly ec2-instance-type=m1.large ec2-region=eu-west-1 > On 2012/03/21 08:33:26, fwereade wrote: > > IMO separating the arguments into constraints/non-constraints is unhelpfully > > icky; set-constraints does currently use raw args, but that's just because > > there's no possibility of ambiguity: they're *all* constraints, rather than > > being a mix of env settings and env sub-settings (as it were). > > Indeed. I hadn't realized that this was mixing both. > > I suggest we have the environment settings as real flags for bootstrap. For > example, in the above case: > > juju bootstrap --apt-proxy=http://bit.ly \ > --constraints="ec2-instance-type=m1.large" > > This way it's still homogeneous, but in the other direction. > sounds good. > https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... > source/drafts/environment-settings.rst:52: juju get-env > On 2012/03/21 08:19:05, hazmat wrote: > > YAML, with a flag for json. > > Sounds good. I'd even drop json for now. > > > I'm realizing that we probably need another flag or require a key to access > > provider settings. as we get to more multi user envs or delegated usage, > > allowing everyone to retrieve the provider keys by default seems suspect. > > I think it's fine to show whatever they've got access to. Just not showing when > they actually do have access isn't really secure, and when we implement access > control we can easily filter out provider credentials if the user doesn't have > it. > > > Otoh requiring a key always seems heavy handed since its an introspection > tool. > > > > what do you think about a happy medium that provider credentials require a > > --provider flag or arg. This also maps well to the proposed storage mode. > > I feel it's unnecessary. Just show everything for now. When we implement access > control, hide to those who can't see it. sounds good.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... File source/drafts/environment-settings.rst (right): https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... source/drafts/environment-settings.rst:25: An example of juju-set usage:: On 2012/03/20 19:23:51, niemeyer wrote: > s/juju-set/set-env/ This is still pending. https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... source/drafts/environment-settings.rst:27: juju set-env apt-proxy http://us-east-1.ec2.archive.ubuntu.com.s3.amazonaws.com/ubuntu/ On 2012/03/20 19:23:51, niemeyer wrote: > Can we have an equals sign within each option. E.g.: > > juju set-env foo=bar foz=baz > > It'd also be good to have a word about validation. set-env should blow up with > unsupported keys, probably implemented with a schema validator. Still pending. https://codereview.appspot.com/5849054/diff/1/source/drafts/environment-setti... source/drafts/environment-settings.rst:48: juju bootstrap apt-proxy=http://bit.ly ec2-instance-type=m1.large ec2-region=eu-west-1 On 2012/03/21 14:34:13, niemeyer wrote: > On 2012/03/21 08:33:26, fwereade wrote: > > IMO separating the arguments into constraints/non-constraints is unhelpfully > > icky; set-constraints does currently use raw args, but that's just because > > there's no possibility of ambiguity: they're *all* constraints, rather than > > being a mix of env settings and env sub-settings (as it were). > > Indeed. I hadn't realized that this was mixing both. > > I suggest we have the environment settings as real flags for bootstrap. For > example, in the above case: > > juju bootstrap --apt-proxy=http://bit.ly \ > --constraints="ec2-instance-type=m1.large" > > This way it's still homogeneous, but in the other direction. This was also not done as suggested. apt-proxy should also be a flag, with --apt-proxy.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
