[HOUSEKEEPING] Remove/Comment Out resources related to key vault for … #458
Conversation
dennisseah
requested review from
edaena,
sarath-p,
andrebriggs and
samiyaakhtar
| { | ||
| "arg": "-k, --key-vault-name <key-vault-name>", | ||
| "description": "Name of the Azure key vault" | ||
| }, | ||
| { | ||
| "arg": "--service-principal-id <service-principal-id>", |
There was a problem hiding this comment.
Are service-principal-id, service-principal-password, tenant-id and subscription-id still being used elsewhere?
There was a problem hiding this comment.
in the onboard command these are used to create account service and table. Isn't this the case?
There was a problem hiding this comment.
The presence of these in config file makes them look mandatory for other commands as well. Perhaps in future we should revisit whether or not the config file should have four of these.
There was a problem hiding this comment.
these are not in config file, these are option values for onboard command only
There was a problem hiding this comment.
they're also in config file: https://github.com/CatalystCode/spk/blob/master/spk-config.yaml#L32
There was a problem hiding this comment.
i missed these, should I remove them to avoid confusion?
There was a problem hiding this comment.
I think for now we can leave them since I believe the onboard command takes them either through config file or through the command itself. Although since it's a one time command, it shouldn't rely on the config file. I'll open a separate discussion for that :)
There was a problem hiding this comment.
The service principal related options are used to authenticate with Azure to create storage account and table if they don't exist. Deleting these options make this command not consistent with other commands to pass all required values without depending on config.yaml.
|
Personally, I prefer removing lines of code that link to things that we don't plan to use since it appears like a user error if things are commented out, but we can leave the keyvault files as is. Does anyone have a preference on how to unlink some code for the moment until we need keyvault again? cc @sarath-p @evanlouie @bnookala |
I will remove the code. I also think that it is right thing to do |
…introspection
closes microsoft/bedrock#1243
@samiyaakhtar, Please let me know if it is ok to comment out code; or you want to remove them. thanks