Skip to content
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

Fix path separator-related bug affecting Windows #13631

Merged
merged 8 commits into from
Jan 9, 2023

Conversation

alopezz
Copy link
Contributor

@alopezz alopezz commented Jan 5, 2023

What does this PR do?

It fixes an errors on Windows caused by the use of a Windows path separator (\) where a / is expected. It also enables Windows tests for the downloader on PR's to try to prevent future regressions.

Motivation

Kitchen tests started to fail for windows, presumably due to #13331. E.g. https://gitlab.ddbuild.io/DataDog/datadog-agent/-/jobs/210470215):

            RuntimeError:
       Failed to install integrations package 'datadog-cilium==2.2.1' - Traceback (most recent call last):
         File "C:\Program Files\Datadog\Datadog Agent\embedded3\lib\runpy.py", line 194, in _run_module_as_main
           return _run_code(code, main_globals, None,
         File "C:\Program Files\Datadog\Datadog Agent\embedded3\lib\runpy.py", line 87, in _run_code
           exec(code, run_globals)
         File "C:\Program Files\Datadog\Datadog Agent\embedded3\lib\site-packages\datadog_checks\downloader\__main__.py", line 9, in <module>
           sys.exit(download())
         File "C:\Program Files\Datadog\Datadog Agent\embedded3\lib\site-packages\datadog_checks\downloader\cli.py", line 124, in download
           wheel_abspath = tuf_downloader.download(wheel_relpath)
         File "C:\Program Files\Datadog\Datadog Agent\embedded3\lib\site-packages\datadog_checks\downloader\download.py", line 266, in download
           return self.__download_with_tuf_in_toto(target_relpath)
         File "C:\Program Files\Datadog\Datadog Agent\embedded3\lib\site-packages\datadog_checks\downloader\download.py", line 253, in __download_with_tuf_in_toto
           self.__download_and_verify_in_toto_metadata(target_relpath, target_abspath, target)
         File "C:\Program Files\Datadog\Datadog Agent\embedded3\lib\site-packages\datadog_checks\downloader\download.py", line 229, in __download_and_verify_in_toto_metadata
           root_layout_abspath, root_layout_target = self.__download_in_toto_root_layout()
         File "C:\Program Files\Datadog\Datadog Agent\embedded3\lib\site-packages\datadog_checks\downloader\download.py", line 129, in __download_in_toto_root_layout
           return self.__download_with_tuf(target_relpath)
         File "C:\Program Files\Datadog\Datadog Agent\embedded3\lib\site-packages\datadog_checks\downloader\download.py", line 105, in __download_with_tuf
           raise TargetNotFoundError(f'Target at {target_relpath} not found')
       datadog_checks.downloader.exceptions.TargetNotFoundError: Target at in-toto-metadata\5.core.root.layout not found

It turns out we're not testing on Windows on CI, so this might have been caught before merge, but wasn't.

Additional Notes

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached
  • If the PR doesn't need to be tested during QA, please add a qa/skip-qa label.

@codecov
Copy link

codecov bot commented Jan 5, 2023

Codecov Report

Merging #13631 (9a0b462) into master (89ae8f4) will increase coverage by 0.00%.
The diff coverage is 90.90%.

Flag Coverage Δ
datadog_checks_downloader 79.86% <90.90%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

`get_target_info` expects a path-relative url
string (https://url.spec.whatwg.org/#path-relative-url-string). `os.path.join`
gives us the wrong separator (`\` instead of `/`). This changes that
to use explicit concatenation to ensure the right separator is used.
@alopezz alopezz changed the title Run downloader tests on windows Fix path separator-related bug affecting Windows Jan 5, 2023
Since we're mixing `\` and `/` anyway due to how some parts of the
path are built manually and others with os.path.join, we can make
testing easier by always returning the same format regardless of
platform; this also makes the downloader's CLI-based API more
consistent regardless of platform.
@@ -335,7 +339,7 @@ def _do_download(package, version=None, root_layout_type="core"):
# -vvvv: INFO
# -vvvvv: DEBUG
cmd = [
"python",
sys.executable,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this change was necessary so that it uses the right python in Windows, otherwise it would be missing the dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afaik this is also what the python docs recommend, so +1 on the change!

@alopezz alopezz marked this pull request as ready for review January 5, 2023 14:31
@alopezz alopezz requested review from a team and trishankatdatadog as code owners January 5, 2023 14:31
@alopezz alopezz merged commit 095aaa8 into master Jan 9, 2023
@alopezz alopezz deleted the alopez/fix-downloader-windows branch January 9, 2023 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants