CFE-4520: Fixed missing "packages" dir with install --package on fresh cf-remote install

[victormlg]

No description provided.

[jakub-nt]

Maybe it would be slightly better to perform the path existence check in download_package instead of _download_urls?

[olehermanse]

Looking at this commit, it's not clear what difference your change makes mkdir vs os.mkdir look like they're supposed to do the same thing, so I'd expect some further explanation in the commit message.

Did you reproduce the issue in the ticket and confirm that your change fixes the issue? Your commit message could include the error message from before, and the reason why os.mkdir and mkdir are different.

The code which was already there is questionable to begin with; it introduces an unnecessary race condition between checking if the dir exists and creating it (someone could create it between those 2 steps). It's also not very explicit about whether parent dirs should be created, or what should happen if there is a file there instead of a directory.

Instead of:

if not os.path.isdir(urls_dir):
    mkdir(urls_dir)

I would do something like:

# os.makedirs also creates parents
os.makedirs(urls_dir, exist_ok=True)

or:

try:
    # os.makedirs also creates parents
    os.makedirs(urls_dir, exist_ok=True)
except:
    # Handle exceptions here
assert(os.path.isdir(urls_dir))

(That logic could probably be worked into the mkdir() util function as well.)

[victormlg]

After rm -rf packages/:

(.venv) victor-moene@victomoe:~/northern.tech/cf-remote (mkdirpackage)$ cf-remote install --bootstrap dubstep --hub dubstep --package https://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-12131/PACKAGES_HUB_x86_64_linux_debian_12/cfengine-nova-hub_3.26.0-1.debian12_amd64.deb
Traceback (most recent call last):
  File "/home/victor-moene/.venv/bin/cf-remote", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/home/victor-moene/.venv/lib/python3.12/site-packages/cf_remote/main.py", line 591, in main
    exit_code = run_command_with_args(args.command, args)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/victor-moene/.venv/lib/python3.12/site-packages/cf_remote/main.py", line 300, in run_command_with_args
    return commands.install(
           ^^^^^^^^^^^^^^^^^
  File "/home/victor-moene/.venv/lib/python3.12/site-packages/cf_remote/commands.py", line 184, in install
    package, hub_package, client_package = _download_urls(packages)
                                           ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/victor-moene/.venv/lib/python3.12/site-packages/cf_remote/commands.py", line 115, in _download_urls
    os.mkdir(urls_dir)
FileNotFoundError: [Errno 2] No such file or directory: '/home/victor-moene/.cfengine/cf-remote/packages/url_specified'

After change:

python3 cf_remote install --bootstrap dubstep --hub dubstep --package https://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-12131/PACKAGES_HUB_x86_64_linux_debian_12/cfengine-nova-hub_3.26.0-1.debian12_amd64.deb
Downloading package: '/home/victor-moene/.cfengine/cf-remote/packages/url_specified/cfengine-nova-hub_3.26.0-1.debian12_amd64.deb'

[email protected]
OS           : Debian 12
Architecture : x86_64
CFEngine     : Not installed
Binaries     : dpkg, apt

Copying: '/home/victor-moene/.cfengine/cf-remote/packages/url_specified/cfengine-nova-hub_3.26.0-1.debian12_amd64.deb' to '[email protected]'
[54.75.66.152]:+ /home/victor-moene/.cfengine/cf-remote/packages/url_specified/cfengine-nova-hub_3.26.0-1.debian12_amd64.deb -> cfengine-nova-hub_3.26.0-1.debian12_amd64.deb
Installing: 'cfengine-nova-hub_3.26.0-1.debian12_amd64.deb' on '[email protected]'

From my understanding, the origin of the bug is that _download_urls expects "packages" to be already created by another function. When calling os.mkdir, it tries to create "packages/url_specified/", however "packages" hasn't been created yet thus creating an exception.

The difference between os.mkdir and the custom function mkdir, is that mkdir is calling os.makedirs, thus creating parent dirs to the new dir if needed. But calling directly os.makedirs(exist_ok=True) is probably safer.

I found it strange that we created a function for mkdir if it introduces a race condition. I used because I thought it would make sense to reuse something we already made. Code for mkdir:

def mkdir(path):
    if not os.path.exists(path):
        log.info("Creating directory: '{}'".format(path))
        os.makedirs(path)
    else:
        log.debug("Directory already exists: '{}'".format(path))