-
Notifications
You must be signed in to change notification settings - Fork 6
Add wait_until_completion to InfrahubBranchManager.create #142
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
Conversation
infrahub_sdk/branch.py
Outdated
| response = await self.client.execute_graphql(query=query.render(), tracker="mutation-branch-create") | ||
|
|
||
| if not wait_until_completion: | ||
| return BranchData(**response["BranchCreate"]["task"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We cannot return branch object as it has not be created yet, does returning
task_idmake sense? Downside is thatcreatereturn signature becomesBranchData | str - Currently,
background_executionhas no effect but is available sdk side, so this is somehow a small breaking change even though I do not expect of this parameter to be used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We cannot return branch object as it has not be created yet, does returning
task_idmake sense? Downside is thatcreatereturn signature becomesBranchData | str
That's unfortunate. But for now I think we should go with BranchData | str I don't remember if that works on Python 3.9 or if we have to do a Union[BranchData, str]. What we can do is to use an @overload decorator which we use in a few other places. We can use that to indicate the returned data type depending on the input params used.
a671c0d to
da85da4
Compare
infrahub_sdk/branch.py
Outdated
| description: str = "", | ||
| background_execution: bool = False, | ||
| ) -> BranchData: | ||
| wait_until_completion: bool = False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add this parameter for other branch methods (merge, ...)?
ede1137 to
700985d
Compare
8d7a9ac to
bb63b82
Compare
| query = Mutation(mutation="BranchCreate", input_data=input_data, query=MUTATION_QUERY_DATA) | ||
| response = await self.client.execute_graphql(query=query.render(), tracker="mutation-branch-create") | ||
|
|
||
| # Make sure server version is recent enough to support background execution, as previously |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this is so useful because if the server doesn't support wait_until_completion it will blow up when you execute the query 2 lines up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, let's use deprecated background_execution as query input field so query does not break, and we will switch to wait_until_completion the day background_execution is completely removed from the graphql query.
bb63b82 to
e95caeb
Compare
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop #142 +/- ##
===========================================
- Coverage 65.40% 65.32% -0.08%
===========================================
Files 76 76
Lines 6923 6942 +19
Branches 1367 1375 +8
===========================================
+ Hits 4528 4535 +7
- Misses 2026 2037 +11
- Partials 369 370 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
e95caeb to
9105436
Compare
Related PR: opsmill/infrahub#4917