-
Notifications
You must be signed in to change notification settings - Fork 6
Refactor internal store #323
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
Codecov ReportAttention: Patch coverage is @@ Coverage Diff @@
## develop #323 +/- ##
===========================================
+ Coverage 71.32% 71.46% +0.13%
===========================================
Files 87 87
Lines 7871 7990 +119
Branches 1517 1544 +27
===========================================
+ Hits 5614 5710 +96
- Misses 1865 1875 +10
- Partials 392 405 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Tested as replacement of #321, all good for me |
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.
Looks good overall to me, but there seems to be some issues that need to be resolved as highlighted by the infrastructure_edge script. Also I don't think that we should add that extra package.
| return None | ||
|
|
||
| if kind and found_invalid: | ||
| raise NodeInvalidError( |
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.
This error is raised when running infrahub_edge.py would be good with a pipeline in the Infrahub repo for this PR.
infrahub_sdk/store.py
Outdated
| we need to save them in order to reuse them later to associate them with another node for example. | ||
| """ | ||
|
|
||
| def __init__(self, default_branch: str = "main") -> None: |
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.
I think it was like this before but it might be unexpected for users that have configured a different default branch when initializing the SDK and then just use these store objects where we default to "main".
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.
I removed the default branch for now but it will introduce a "breaking change" so not 100% sure if we should go with it
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.
No with that in mind I'd suggest to keep it as is and instead create an issue so that we cycle back and refactor this in a controlled manner.
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.
I added the default branch back along with a deprecation warning
1541e27 to
4c7c2a7
Compare
|
Based on the changes done in |
I will add a fragment If we add back the default branch I think it should work out of the box for existing users but I asked the CS team to do a quick check just to be sure |
4c7c2a7 to
daa6fdf
Compare
This PR refactor the internal store to be easier to use and to cover more use cases
The main changes is the introduction of a new
internal_idon all nodes to simplify how the nodes are organized within the store event before have been created in Infrahub.Also with this change the store is now branch-aware and by default objects from 2 different branches will be isolated from each other.
And last but not least, it's now easier to search node based on their HFID both with the flat version or the structured one.