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

image-download: fix handling of COCKPIT_IMAGE_UPLOAD_STORE #6074

Merged
merged 7 commits into from
Mar 18, 2024

Conversation

allisonkarlitskaya
Copy link
Member

@allisonkarlitskaya allisonkarlitskaya commented Mar 12, 2024

These are developer tools and it's far more useful to see errors with
tracebacks instead of pretending that we handle this in a useful way.
We were accidentally modifying these constants from the files that used
them, because this code:

  stores = PUBLIC_STORES
  ...
  stores += REDHAT_STORES

modifies in-place.

Change the stores lists to be constants and fix up our handling.

We also fix our handling of the COCKPIT_IMAGE_UPLOAD_STORE variable in
image-download: the way we were appending it was resulting in each
individual character being added as a separate item, which resulted in
failures when attempting to parse the single-character URLs.

Fixes #6037
Instead of returning (None, None) on failure, just return None.  mypy
finds it easier to reason about that.
...and put us on the strict list.

In order to do that we also needed to annotate testmap and s3, so add
those to the list as well.
pytest thinks that TestLogger is something that it might want to
collect, and issues this warning:

  PytestCollectionWarning: cannot collect test class 'TestLogger' because it has a __init__ constructor (from: test/test_github.py)

Rename the class.
Nothing was running this before, and it even contained an error (the
`lib` import was broken).

Move it together with the rest of our pytest stuff, with a very minor
adjustment to an import path.

Now everything in lib/ passes mypy --strict, so adjust the list.
@martinpitt
Copy link
Member

This uses "too modern" type annotations which don't work on RHEL 8, and image-download is being used there. I get it to work with this diff:

diff --git a/image-download b/image-download
index 149c9346..46b9e1ec 100755
--- a/image-download
+++ b/image-download
@@ -68,7 +68,7 @@ def curl_cmd(args: Sequence[str]) -> Sequence[str]:
     return ['curl', '--connect-timeout', '10', '--fail', *args]
 
 
-def check_curl_args(args: Sequence[str]) -> tuple[str, float] | None:
+def check_curl_args(args: Sequence[str]) -> 'tuple[str, float] | None':
     head_args = ['--silent', '--head']  # only used for this check
 
     try:
@@ -80,7 +80,7 @@ def check_curl_args(args: Sequence[str]) -> tuple[str, float] | None:
         return None
 
 
-def find(name: str, stores: Sequence[str], latest: float, quiet: bool) -> tuple[list[str], str] | None:
+def find(name: str, stores: Sequence[str], latest: float, quiet: bool) -> 'tuple[list[str], str] | None':
     found = []
 
     for store in stores:
@@ -111,7 +111,7 @@ def find(name: str, stores: Sequence[str], latest: float, quiet: bool) -> tuple[
         return None
 
     # Find the most recent version of this file
-    def header_date(args: tuple[Sequence[str], tuple[str, float], str]) -> str | float:
+    def header_date(args: 'tuple[Sequence[str], tuple[str, float], str]') -> 'str | float':
         _, (output, duration), message = args
         try:
             reply_line, headers_alone = output.split('\n', 1)
diff --git a/lib/s3.py b/lib/s3.py
index 1768b7a8..2f5940a1 100644
--- a/lib/s3.py
+++ b/lib/s3.py
@@ -43,7 +43,7 @@ SHA256_NIL = hashlib.sha256(b'').hexdigest()
 logger = logging.getLogger('s3')
 
 
-def get_key(url: urllib.parse.ParseResult) -> tuple[str, str]:
+def get_key(url: urllib.parse.ParseResult) -> 'tuple[str, str]':
     s3_key_dir = xdg_config_home('cockpit-dev/s3-keys', envvar='COCKPIT_S3_KEY_DIR')
 
     if url.hostname is None:
@@ -75,7 +75,7 @@ def is_key_present(url: urllib.parse.ParseResult) -> bool:
 
 def sign_request(
     url: urllib.parse.ParseResult, method: str, headers: Mapping[str, str], checksum: str
-) -> dict[str, str]:
+) -> 'dict[str, str]':
     """Signs an AWS request using the AWS4-HMAC-SHA256 algorithm
 
     Returns a dictionary of extra headers which need to be sent along with the request.
diff --git a/lib/testmap.py b/lib/testmap.py
index 232dba97..94e523c7 100644
--- a/lib/testmap.py
+++ b/lib/testmap.py
@@ -278,7 +278,7 @@ def get_test_image(image: str) -> str:
     return image.replace("-distropkg", "")
 
 
-def split_context(context: str) -> tuple[str, int | None, str, str]:
+def split_context(context: str) -> 'tuple[str, int | None, str, str]':
     bots_pr = None
     repo_branch = ""
 

For the simple cases you can also use Tuple/Dict from typing, but I suppose just quoting them is simpler all around.

Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Cheers! This looks good of course, but needs some "ancient Python" treatment. I'll trigger a cockpit-machines test to demonstrate.

@martinpitt
Copy link
Member

I'll trigger a cockpit-machines test to demonstrate.

Ah no, that won't work -- this only happens on Testing Farm. Perhaps we should add a little c8s workflow run.. (not necessarily in this PR)

@allisonkarlitskaya
Copy link
Member Author

This is blocked until we get all testing farm runs moved over to using the tasks container as we did in cockpit-project/starter-kit#812.

@martinpitt
Copy link
Member

Note: this is only really blocked on cockpit-machines. The others should be fine. Adding cockpit-project/cockpit-machines#1502 to todo list

Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Aside from the blocker, the code changes look great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

image-download: incorrect handling of COCKPIT_IMAGE_UPLOAD_STORES
2 participants