Skip to content

Conversation

@ogenstad
Copy link
Contributor

@ogenstad ogenstad commented Nov 6, 2024

This work relates to opsmill/infrahub#3932 as a goal to have a consistent way of loading modules that behaves the same way. An issue that might come up is that of circular imports as the check.py is importing InfrahubClient.

We're removing the use of InfrahubCheck.init() from the CTL as it doesn't provide much value and also marking that method as deprecated.

@codecov
Copy link

codecov bot commented Nov 6, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
infrahub_sdk/checks.py 66.66% 2 Missing and 1 partial ⚠️
infrahub_sdk/ctl/check.py 66.66% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (efdcfb7) and HEAD (e231c93). Click for more details.

HEAD has 4 uploads less than BASE
Flag BASE (efdcfb7) HEAD (e231c93)
python-3.10 1 0
python-3.9 1 0
python-filler-3.12 1 0
python-3.11 1 0
@@             Coverage Diff              @@
##           develop     #126       +/-   ##
============================================
- Coverage    65.38%   44.43%   -20.96%     
============================================
  Files           76       76               
  Lines         6917     6922        +5     
  Branches      1367     1296       -71     
============================================
- Hits          4523     3076     -1447     
- Misses        2026     3566     +1540     
+ Partials       368      280       -88     
Flag Coverage Δ
python-3.10 ?
python-3.11 ?
python-3.12 44.43% <66.66%> (-0.01%) ⬇️
python-3.9 ?
python-filler-3.12 ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
infrahub_sdk/ctl/check.py 27.00% <66.66%> (ø)
infrahub_sdk/checks.py 28.20% <66.66%> (-35.19%) ⬇️

... and 41 files with indirect coverage changes

@ogenstad ogenstad marked this pull request as ready for review November 6, 2024 13:47
@ogenstad ogenstad requested a review from a team November 6, 2024 13:47
async def init(cls, client: Optional[InfrahubClient] = None, *args: Any, **kwargs: Any) -> InfrahubCheck:
"""Async init method, If an existing InfrahubClient client hasn't been provided, one will be created automatically."""
warnings.warn(
"InfrahubCheck.init has been deprecated and will be removed in the version in Infrahub SDK 1.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code will ship for the first time with 1.1 so this can't be accurate
To be clean let's say 2.0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to get rid of the InfrahubClient() call below as it requires us to specifically import InfrahubClient. Are you fine with saying "after 1.1.0" instead? Alternatively we can change it to 2.0 and use importlib or similar here until that point. I don't think anyone outside of infrahubctl or the git-agent ever used this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed an update to schedule this method to be removed in SDK 2.0 instead and at the same type we change how InfrahubClient is used within that module in order to change the InfrahubCheck module to follow the same logic as used for Transforms and generators in #128.

@ogenstad ogenstad force-pushed the pog-check-init branch 2 times, most recently from 674ad9c to 7031c18 Compare November 7, 2024 13:16
@ogenstad ogenstad merged commit 386440d into develop Nov 7, 2024
10 checks passed
@ogenstad ogenstad deleted the pog-check-init branch November 7, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants