Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add context command #1655
Add context command #1655
Changes from 33 commits
818448b
5817e65
5e6726d
3c2ad43
5603417
608a3d3
f3ba5a5
a1feb76
11fdc19
5a79eaf
f6071f5
e120990
38fd39d
03456b4
3981a83
5f62656
a64a1e2
8da10c5
e6d6017
afd2001
eedd004
35dc59f
bb144b0
e6c8499
e331299
d12c403
087ec8d
67b1ff4
1d85e25
7805f45
ac99832
878573c
4bd4914
b72874e
6b31ea4
84dd4e2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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 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 isdsh 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, likedsh context remove
. But fordsh context switch
you supply thename
, which is also described in the help as thename of the context to switch to
. So sometimes KEY is the name of the context, and sometimes NAME is the name of the context. Fordsh context switch
you actually need to supply thekey
rather than the name.key
andname
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?dsh context create
anddsh context update
could provide a bit more info about what they do.dsh context update
does nothing if no flags are set.location
fromuksouth
toukwest
- 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 -context
commands, as existed forinit
?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 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.
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]
vscontexts[key]
. And you would add the additional restriction that names also need to be unique, even if they are deployed to different subscriptions/locations.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.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 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.
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.
Helps!
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.
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.
I still just think it's quite confusing. If I run
dsh context add <foo>
, then rundsh context create
, then rundsh context update <foo>
, then rundsh 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 beforedsh context create
. Which isn't necessarily a problem, just, again, feel like it should be more explicit.On reflection, I think I'm ok with this given we're mostly expecting these arguments to get recorded in config files now anyway.
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, 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.
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 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.
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, 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".