fix(registries): query registries directly by organization when possible #10991
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
f34cb8a to
af4d7dd
Compare
pingleiwandb
left a comment
There was a problem hiding this comment.
LGTM, just nit and my python noob questions (as usual 🐶 )
| if server_supports(client, pb.REGISTRIES_ON_ORGANIZATION) and (not filter): | ||
| self.QUERY = gql(gen.FETCH_REGISTRIES_GQL) | ||
| self._result_cls = gen.FetchRegistries | ||
| else: |
There was a problem hiding this comment.
curious why the the legacy branch is the one not covered by tests? I would assume existing tests on (not released) server image would trigger the old branch because server_supports would return false and fall into this branch.
Or the coverage is from mocks?...
There was a problem hiding this comment.
I'm guessing it's at least partly due to the failing tests, which wouldn't count toward test coverage IIRC. Though TBD (need to take a closer look and fix the tests either way once I get a chance)
| TOTAL_COUNT_IN_FILE_CONNECTION = 15; | ||
|
|
||
| // Indicates that the server supports querying for registries on an organization. | ||
| REGISTRIES_ON_ORGANIZATION = 16; |
There was a problem hiding this comment.
Just curious, we define sever feature flag on server without proto, how did it endup being proto on SDK side?
There was a problem hiding this comment.
Yeah, I've noticed this too but it's probably better if I defer to the SDK team for more context on the original implementation
(If I had to guess, it might have been needed for compatibility with sdk-core, but treat this as pure speculation that could be wrong)
| if not ((org := result.organization) and (org_entity := org.org_entity)): | ||
| result = self._result_cls.model_validate(data) | ||
|
|
||
| # FIXME: Tighten this up a bit, this results in `conn` being unbound in the `try` block below. |
There was a problem hiding this comment.
Maybe we just add a final else to throw some internal error because conn is not set when we server return some invalid response?
btw: (not related to the feature itself), is there a way in python (library) to make the chain of check easier? Similar to js's optional chaining? org?.org_entity?.projects
There was a problem hiding this comment.
btw: (not related to the feature itself), is there a way in python (library) to make the chain of check easier? Similar to js's optional chaining? org?.org_entity?.projects
Not really AFAIK but it's a good question and I want something like this so much (so does a lot of the Python community, I'm sure).
The closest I've seen is the returns library, which defines a Maybe container (along with Some/Nothing types) that may look familiar from other languages.
Main downsides of using it here --
- it's a whole extra added dependency
- it's still kind of verbose
- if you're not already familiar with it, it requires extra learning to get familiar with how -- and how not -- to use it. (pydantic has this tradeoff too ofc, but I think pydantic has much more immediate benefits)
- It doesn't always play well with Python type hints / type checks. Not really the library's fault, they've actually done a lot to make sure the library is as fully typed as possible (there are just inevitable corner cases that Python's typing still can't express/handle well for now)
Description
Ensure the GraphQL operation called by
Api.registries()uses the newOrganization.registriesfield when available + applicable.For now, we still have to fall back on the prior query (i.e. selecting
Organization.orgEntity.projects(...)) when:Organization.registriesfilterson registry projects. Note thatOrganization.registries(...)does not yet accept afilters: JSONStringarg.Testing
How was this PR tested?