Dan/per 12033 cherrypick changes from pdp 070 #245
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reimplementation of #219
(cherry picked from commit 45147fd)
Reimplementing #224
Reimplementation of #225
Reimplementation of #226
(cherry picked from commit 0073bae)
(cherry picked from commit c44c215)
Reimplementation of #229
(cherry picked from commit 1744bc6)
* update helm chart version * workflow change (cherry picked from commit 89cb8f7)
* Fixed state file deadlock * Fixed pre-commit (cherry picked from commit f00a665)
* update pdp api example on ecs * check ecs * final - update our internal pdp (cherry picked from commit 08eab87)
(cherry picked from commit e27a27a)
Reimplementation of #232
(cherry picked from commit a0f224c)
(cherry picked from commit fffb1c0)
(cherry picked from commit 73d0c19)
* Added upgrade pip to docker image * Updated OPAL to v0.8.0rc1 (cherry picked from commit 3a74fdf)
omer9564
reviewed
Mar 20, 2025
omer9564
reviewed
Mar 20, 2025
omer9564
reviewed
Mar 20, 2025
omer9564
reviewed
Mar 20, 2025
omer9564
reviewed
Mar 20, 2025
omer9564
reviewed
Mar 20, 2025
| mapping_rules_json = [] | ||
| else: | ||
| mapping_rules_json = data_result.get("all") | ||
| mapping_rules_json = data_result.get("all", []) if data_result is not None else [] |
There was a problem hiding this comment.
if data_result is
{
"all": null
}then the mapping_rules_json will be None
There was a problem hiding this comment.
There was a discussion about it back then, I can't remember what the understanding was.
#226 (comment)
What would you suggest doing to make it clearer?
There was a problem hiding this comment.
I guess if we already talked about it then it's redundant to talk again
omer9564
reviewed
Mar 20, 2025
omer9564
reviewed
Mar 20, 2025
omer9564
reviewed
Mar 20, 2025
omer9564
reviewed
Mar 20, 2025
Comment on lines
+161
to
+166
| except Exception as e: | ||
| raise RelayAPIError( | ||
| "relay-api", | ||
| response.status, | ||
| f"Server responded to token request with a bad status: {text}", | ||
| ) from e |
There was a problem hiding this comment.
[CHANGE] this isn't exactly the same behavior, before the change it always raised, not it doesn't always raise
omer9564
reviewed
Mar 20, 2025
omer9564
reviewed
Mar 20, 2025
horizon/state.py
Outdated
Comment on lines
214
to
217
| if sdk not in self._state.seen_sdks: | ||
| # ensure_future is expensive, only call it if actually needed | ||
| asyncio.ensure_future(self._report_seen_sdk(sdk)) | ||
| async with self._write_lock: | ||
| self._tasks.append(asyncio.create_task(self._report_seen_sdk(sdk))) |
There was a problem hiding this comment.
wasn't this later removed or somehting like that ? in favor of simply calling the report_seen_sdk
omer9564
reviewed
Mar 20, 2025
| @@ -1,10 +1,4 @@ | |||
| import os | |||
|
|
|||
| from opal_common.logger import logger | |||
There was a problem hiding this comment.
The code was removed but the import remained
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.