Conversation
Signed-off-by: Juan Pasutti <juanpasutti@gmail.com>
| if self.config_key is not None: | ||
| keys += [self.config_key] | ||
| if self.config_key is not None: # Leave out None values | ||
| if self.config_key.strip(): # Leave out empty strings | ||
| keys += [self.config_key] | ||
| else: | ||
| self.logger.warning("GitHub API key is an empty string. Please, add a valid one.") |
There was a problem hiding this comment.
hmm i wonder if we could shorten this to
if self.config_key:
(and keep the else block)
AFAIK (needs testing) i think python treats empty strings as falsy
There was a problem hiding this comment.
I tried this approach before the actual nested implementation. The three cases are pinned in the test file ("", " ", None)
The problem is that if self.config_key: would catch None and "", but " " is not falsy, so it gets added . With a flat else, None logs the warning, which test_none_config_key_with_no_db_keys asserts it shouldn't: None just means the variable was never set.
Also tried if self.config_key.strip(). Catches de whitespace but raises an AttributeError on None.
There was a problem hiding this comment.
do you think it would it be better to throw a self.config_key = self.config_key.strip() earlier in the process? then it would collapse the whitespace case into the "" case that is already handled with if self.config_key?
There was a problem hiding this comment.
Yes, probably the besst approach.
I'll add
if self.config_key:
self.config_key = self.config_key.strip()
right after the key is read with self.config_key = self.get_config_key(). Then, this part can end up as:
if self.config_key:
keys += [self.config_key]
elif self.config_key is not None: # None just means it was never set
self.logger.warning("GitHub API key is an empty string. Please, add a valid one.")
That elif for None needs to be there so we don't log a warning for a missing token.
Note: self.config_key is also read to filter the config key out of the database keys, so that comparison now will see the normalized value. I think that's an improvement
There was a problem hiding this comment.
right after the key is read with self.config_key = self.get_config_key()
would it help to log a warning that we handled unnecessary whitespace in a key in case the admin wants to remove it?
Signed-off-by: Juan Pasutti <juanpasutti@gmail.com>
id say for this if you update your pr description to say "fixes part of" the referenced issue (break the automatic closure that will otherwise happen when this merges) and then we can have the gitlab version in another PR. |
Signed-off-by: Juan Pasutti <juanpasutti@gmail.com>
ba479f3 to
5e74418
Compare
Description
When
COLLECTOSS_GITHUB_API_KEYenv variable was set to one or more whitespaces or left empty, that counted as set config_key, causinghttpx.LocalProtocolError: Illegal header value b'token '.Now, in
GithubApiKeyHandler.get_api_keys():None, the warning is not logged and doesn't add the key (already covered before the fix).Added
tests/test_classes/test_github_api_keys.pycovering the config key cases:empty and whitespaces with and without database keys,
None, and a valid key. Each oneasserts the key list, if the key is probed, and the warning.
docker compose up --buildlogs after the fix:This PR fixes part of #378
Notes for Reviewers
GitlabApiKeyHandler.get_api_keys(). I didn't add that fix here, just GitHub's. Let me know if I can open up a follow-up PR, open an issue (or both) or add the code and tests in this same PR.Signed commits
Generative AI disclosure
Please select one option:
If AI tools were used, please provide details below:
- What tools were used? Claude
- How were these tools used? Code review and tests guidance (not writing, suggested new ones covering cases I didn't consider)
- Did you review these outputs before submitting this PR? Yes, all the code was written by me, trying different approaches and Claude was used to review the changes.