Fix GraphQL query sent with hierarchical nodes #149
Conversation
LucasG0
force-pushed
the
lgu-fix-node-hierarchical
branch
from
8e85fe6 to
88ad1c7
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #149 +/- ##
===========================================
+ Coverage 65.32% 65.36% +0.03%
===========================================
Files 76 76
Lines 6942 6950 +8
Branches 1375 1377 +2
===========================================
+ Hits 4535 4543 +8
Misses 2033 2033
Partials 374 374
Flags with carried forward coverage won't be shown. Click here to find out more.
🚨 Try these New Features:
|
LucasG0
force-pushed
the
lgu-fix-node-hierarchical
branch
from
88ad1c7 to
5ff89a9
Compare
LucasG0
changed the title
|
This looks good to me, it might be a bit weird that we start to use fragments without actually requesting fragments. I.e. even if someone did this: from infrahub_sdk import InfrahubClientSync
client = InfrahubClientSync()
nodes = client.all(kind="LocationSite", prefetch_relationships=True, populate_store=True, fragment=False)We'd still use fragments which might seem confusing. Having said given how these hierarchial models work I think it's a good compromise and most users wouldn't care about the fact that these objects require fragments. Thoughts @gmazoyer? It would be good to have a pipeline complete with this change in the main infrahub repo. Can you open a PR there that updates the submodule to this commit? |
Actually the idea of using a fragment came up during our quick sync with. We thoughts that going down that path would simplify the code (instead of going through several if clauses) and probably would not hurt the user in the process. So I agreed with Lucas' choice, and thought it was a pretty good idea. |
| @@ -293,3 +293,28 @@ async def test_create_branch(self, client: InfrahubClient, db: InfrahubDatabase, | |||
| async def test_create_branch_async(self, client: InfrahubClient, db: InfrahubDatabase, init_db_base, base_dataset): | |||
| task_id = await client.branch.create(branch_name="new-branch-2", wait_until_completion=False) | |||
| assert isinstance(task_id, str) | |||
|
|
|||
| # See issue #148. | |||
There was a problem hiding this comment.
I asked myself this once: should we just put the entire link to the issue as a comment to a test that solves it? I think it makes sense to have it/
There was a problem hiding this comment.
It depends on our preferencies, actually, I think I prefer full link too. It's a bit more verbose but it allows quicker access to corresponding issue, and it is even more relevant as we have now multiple repositories (at least sdk/server), and it should be easy to find/replace these links in case repositories names change.
|
Main infrahub repo related PR: opsmill/infrahub#5023 |
No description provided.