Remove / move `ensure_dir` call since it might cause some issues and not consistent

[shcheklein]

This code:

https://github.com/microsoft/knack/blob/dev/knack/config.py#L39-L44

is causing a bunch of potential issues:

1. If it's using the _DEFAULT_CONFIG_DIR that is trying to expand ~ it will break on systems where a system user is used w/o a home dir. Error will look like Permission Error /nonexistent (~ expands into nonexistent on such systems). I see that it's regular a practice to use os.path.expanduser(os.path.join('~', '.{}'.format('cli'))) or similar across Azure tools, but it's causing these errors.

2. If config dir is taken from an env variable down the code, we never run ensure_dir on it (at least on read), which might also cause same issues as were the reason to add it in the first place?

For the context, it was added here https://github.com/microsoft/knack/pull/91. Discussion is here https://github.com/Azure/azure-cli/issues/5001#issuecomment-347993886 . @derekbekoe may be you can give some insights on why it was the best place to resolve the initial issue, I don't have enough context to connect the dots yet.

I think we can try to do it in way that it's not needed for readonly mode (it just means that config is empty and tries to use env), for writes it should try to create it a fail. I see that we already have in:

https://github.com/microsoft/knack/blame/dev/knack/config.py#L223-L224

[shcheklein]

Btw, I can try to contribute a fix back. Just curious about the bigger picture / context so that we don't break some upstream dependencies that already expect this behavior (why?).

[derekbekoe]

Disclaimer: It's been several years since I last worked on this project.

However, reading through your analysis, I'm not sure why in https://github.com/microsoft/knack/pull/91/files, it was added where it was. Yes it seems it'd be best to have the call to ensure_dir() in CLIConfig#init happen a few lines down after self.config_dir is defined since that's the directory that will be used.

[shcheklein]

Yes it seems it'd be best to have the call to ensure_dir() in CLIConfig#init happen a few lines down after self.config_dir is defined since that's the directory that will be used.

Thanks @derekbekoe ! That's right it would be better (I think). But I don't quite understand the need for it anyways ... it would be still breaking some scenarios even a few lines below AFAI can tell.

[derekbekoe]

Removing the check altogether may make sense.

One consideration is ensure_dir(config_dir) may have been added in CLIConfig#init to help keep the check central vs. needing to have to check if the directory exists in multiple other places. For example, if someone is storing other files in the same config directory and wants to access those files directly and outside of the config methods provided by this library. Without the central ensure_dir(...) they'd have to handle this check themselves each time beforehand.

Removing that line could be a breaking change for people that depended on this library doing that check for them. Azure CLI does in-fact do its own check anyway though - https://github.com/Azure/azure-cli/blob/aee788b9c61b7982a5c8e7807d86d764c7f7a798/src/azure-cli-core/azure/cli/core/__init__.py#L78C31-L78C31.

Just something else to consider.

[shcheklein]

Thanks @derekbekoe for the research. Since it's a small change I would leave this decision to maintainers (I can do a PR if that makes sense).

Azure CLI does in-fact do its own check anyway though

I've seen that it is also using os.path.expanduser(os.path.join('~', '.{}'.format('cli'))) -like logic to determine the default path, but I didn't see that call. It means most likely that it'll keep breaking in the environments where home dir doesn't exist.

E.g. I've tried pip install in the Dcoker that has something like:

RUN  addgroup --system dvc && \
          adduser --system  --ingroup dvc dvc && \
USER dvc

And even pip install was breaking for me with a similar error. I didn't check the full stack trace though.