Add context command

[JimMadge]

✅ Checklist

• You have given your pull request a meaningful title (e.g. Enable foobar integration rather than 515 foobar).
• You are targeting the appropriate branch. If you're not certain which one this is, it should be develop.
• Your branch is up-to-date with the target branch (it probably was when you started, but it may have changed since then).
• You have marked this pull request as a draft and added '[WIP]' to the title if needed (if you're not yet ready to merge).
• You have formatted your code using appropriate automated tools (for example ./tests/AutoFormat_Powershell.ps1 -TargetPath <path to file or directory> for Powershell).

⤴️ Summary

• Replace Backend with Context (context to avoid confusion with Pulumi backend)
• Replace init and teardown backend commands with the context command group
• Add additional commands to help manage contexts (add, remove, update, show, switch, available)
• Move init arguments to ContextSettings configuration (editable in the file, or with context add, context update, context remove)
• Configuration supports multiple contexts, and switching between them
• Introduce testing framework, add tests for Context/ContextSettings classes and context commands

Context is overloaded in the codebase. There is the ContextSettings and Context (local data giving the minimum information needed to connect to the shared infrastructure for managing DSH) and Context and ConfigSectionContext (a class for managing the context infrastructure, and corresponding configuration data used in operations later).
It would be good to clarify this to avoid confusion.

Adding tests has revealed a lot of warning are emitted when running dsh, mostly deprecation warnings for pulumi-azure modules.
I think they are otherwise suppressed, would be worth keeping an eye on them.

Todo

• Fix tests in CI (they pass locally, may be due to package version differences)
• Check bootstrapping contexts configuration works
• Update usage guide

🌂 Related issues

Closes #1651
Contributes to #1640

🔬 Tests

• Creating/removing contexts
• Unit tests

[craddm]

Note that dsh deploy shm doesn't work after dsh context create, presumably because the context does not include the aad_tenant_id

[craddm]

I like this in general but it raises quite a few questions. Some of this is me perhaps overthinking it worrying about ways people might mess things up, and wondering how much we should stop them from doing that.
The docs/purpose of the functions could use a bit of clarification and checking for consistency.

We start by adding a context. When adding a context, you give a key - the command is dsh context add [OPTIONS] KEY - which is described as the "name of the context to add". But there's also a --name argument, which is the "human friendly" name of the deployment. For some functions you then supply the key, like dsh context remove. But for dsh context switch you supply the name, which is also described in the help as the name of the context to switch to. So sometimes KEY is the name of the context, and sometimes NAME is the name of the context. For dsh context switch you actually need to supply the key rather than the name.

1. I assume you're separating key and name deliberately - is it helpful to separate the context name and the deployment name, or is it better to just give a single name? e.g. You could set up two otherwise identical contexts with different keys, but why?
2. I guess these will be addressed when we do the complete website docs, but the help info for dsh context create and dsh context update could provide a bit more info about what they do.
3. Is it possible to get Typer to complain if no flags are passed? e.g. dsh context update does nothing if no flags are set.
4. What is the workflow after updating the context? Suppose I update the location from uksouth to ukwest - what should happen when I run another command? If I already have something deployed, should it then deploy new things in the new location? At the moment, running, say, dsh context create after updating the location actually just seems to check the existing location -

5. Could you add short form flags for each argument to the various context commands, as existed for init?

[JimMadge]

Agreed about name/key and update confusion.
I've updated the help descriptions and argument names in 84dd4e2
Do you think that is enough? I think we need to balance keeping the CLI help messages brief, and can put a more detailed explanation in the docs.

You could set up two otherwise identical contexts with different keys, but why?

That is true, it isn't the intention but we don't prevent a user from making conflicting/overlapping contexts.
We could do that, but I think that would want to be a new issue as it is a new feature.

A context has a parameter called "name" and we refer to contexts by their keys, which must be unique. You could use a parameter of each context, but it would be awkward (and more expensive) to select them [c if c.name == name for c in contexts][0] vs contexts[key]. And you would add the additional restriction that names also need to be unique, even if they are deployed to different subscriptions/locations.

Is it possible to get Typer to complain if no flags are passed? e.g. dsh context update does nothing if no flags are set.

Probably, or at least you could raise an exception if all are None. I'm not sure if I see a strong advantage, I think saying "update nothing" and nothing changing makes sense.

What is the workflow after updating the context? Suppose I update the location from uksouth to ukwest - what should happen when I run another command? If I already have something deployed, should it then deploy new things in the new location? At the moment, running, say, dsh context create after updating the location actually just seems to check the existing location

I think that must also have been true before as I haven't touched that code.
It would be nice if context create were idempotent, but as we are using the Azure Python SDK if would be a fair bit of effort.
I've updated the description to say that update updates the settings specifically.
If we want to change/explain this I think it can be a new issue.

Could you add short form flags for each argument to the various context commands, as existed for init?

Could do. I intentionally didn't though.
I feel that the choices have fairly large consequences, so using clear names rather than letters (or even worse, positional arguments!) helps make sure we understand what each argument is.

[craddm]

Agreed about name/key and update confusion. I've updated the help descriptions and argument names in 84dd4e2 Do you think that is enough? I think we need to balance keeping the CLI help messages brief, and can put a more detailed explanation in the docs.

Helps!

You could set up two otherwise identical contexts with different keys, but why?

That is true, it isn't the intention but we don't prevent a user from making conflicting/overlapping contexts. We could do that, but I think that would want to be a new issue as it is a new feature.

A context has a parameter called "name" and we refer to contexts by their keys, which must be unique. You could use a parameter of each context, but it would be awkward (and more expensive) to select them [c if c.name == name for c in contexts][0] vs contexts[key]. And you would add the additional restriction that names also need to be unique, even if they are deployed to different subscriptions/locations.

More awkward/expensive may be true, but cost doesn't seem like an issue here really - it's just searching through a very small list, once in a while. In the context of a deployment, it's the blink of an eye.

Is it possible to get Typer to complain if no flags are passed? e.g. dsh context update does nothing if no flags are set.

Probably, or at least you could raise an exception if all are None. I'm not sure if I see a strong advantage, I think saying "update nothing" and nothing changing makes sense.

Perhaps it's me - I feel like it should be explicit that nothing is changing. Why write a new config file if nothing has changed? Not something I feel strongly about I guess.

What is the workflow after updating the context? Suppose I update the location from uksouth to ukwest - what should happen when I run another command? If I already have something deployed, should it then deploy new things in the new location? At the moment, running, say, dsh context create after updating the location actually just seems to check the existing location

I think that must also have been true before as I haven't touched that code. It would be nice if context create were idempotent, but as we are using the Azure Python SDK if would be a fair bit of effort. I've updated the description to say that update updates the settings specifically. If we want to change/explain this I think it can be a new issue.

I still just think it's quite confusing. If I run dsh context add <foo>, then run dsh context create, then run dsh context update <foo>, then run dsh context create again, nothing changes:

It reads project settings from the context file, but does nothing with them. It just uses the config that already exists without changing it, then re-uploads it at the end, unchanged. So it seems dsh context update is only useful before dsh context create. Which isn't necessarily a problem, just, again, feel like it should be more explicit.

Could you add short form flags for each argument to the various context commands, as existed for init?

Could do. I intentionally didn't though. I feel that the choices have fairly large consequences, so using clear names rather than letters (or even worse, positional arguments!) helps make sure we understand what each argument is.

On reflection, I think I'm ok with this given we're mostly expecting these arguments to get recorded in config files now anyway.

[JimMadge]

Ok, can you give a steer on what (if anything) you think should be changed at how?

Some of these things are (I feel) outside the scope of this issue, being in code that wasn't touched here. We can open issues if we feel like those are worth doing though.

I'm fairly strongly objected to removing keys for contexts. Removing a unique identifier feels like asking for trouble.

[craddm]

So I think what I'm trying to get my head round is what is changing the local config file meant to achieve? So if the purpose here is just to get a command in that changes the local config file, the code as is undoubtedly does that.

Whether what I'm wondering about it involves changes to this code or other code depends a little on what changing the config file is supposed to achieve. Currently the only use for changing the local config file seems to be changing it when nothing is deployed or before tearing down and redeploying existing resources.

I feel like either this code should say something like "You already have resources deployed, changes made here will not have an effect until the resources are deleted and redeployed", or the other code should say something like "The local config differs from existing resources, maybe you should redeploy". Or perhaps these are things that should be in accompanying documentation.

I think I'm just trying to work out when it is meant to be useful to change the local config file in this way, and thus whether it's useful for the new code here to explicitly inform people that what they are doing needs additional steps to be actually useful.

[JimMadge]

So if the purpose here is just to get a command in that changes the local config file, the code as is undoubtedly does that.

Yes, just that and nothing more 😄.

The way I'm seeing this is like git, git remotes in particular.
The context configuration file is a specification of what contexts exist and enough information to connect to them.
Similar to how git remotes are a record of repositories.
I think it is also important to understand that these settings are only for the context infrastructure, not for the TRE itself.

I don't think we should want editing the record of contexts to make changes to any infrastructure, or create warnings?
Also, I'm not certain it is possible.
How do you know if resources were already deployed or if local differs from remote?
All the program knows is the data in the contexts file.

I could see the argument to just remove the update command as it might not be much use.
Maybe it is only really useful in some rare situations like "I made one mistake when running dsh context create" or "I decided to change a parameter before deploying the context infrastructure".

[JimMadge]

Note that dsh deploy shm doesn't work after dsh context create, presumably because the context does not include the aad_tenant_id

This isn't a regression, it was true before. The --aad-tenant-id argument is provided to dsh deploy shm.
That is something I want to change in #1640 though.

So, I would say leave that for another PR.

[craddm]

Agreed that this does what it needs to do for now!