microsoft / microsoft/knack

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

Open
#277 5 comments 2 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
358
Forks
101
PR merge metrics
No merged PRs in 30d

Description

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

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with knack/config.py lines 39-44, then compare the existing handling at lines 223-224. Review pull request #91 and the linked Azure CLI discussion for the original rationale, and verify the desired behavior for read-only configuration, environment-provided paths, and writes that need directory creation.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
cli
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.