Changes to prevent the use of method find_dotenv

[nithinb]

I have added information as part of the Jira ticket itself.

Primarily we want to avoid the usage of find_dotenv since it recursively traverses to root in search of .env file. There is no way for us to limit this search. It is safer measure to stop looking if we don't find anything in the current directory.

This will involve a major version bump primarily because I think this is a breaking change and needs to be communicated accordingly to customers as well.

[codecov]

Codecov Report

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

Project coverage is 75.25%. Comparing base (050b6fe) to head (5f3fdc6).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
cognite/extractorutils/base.py	20.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #429      +/-   ##
==========================================
+ Coverage   75.22%   75.25%   +0.02%     
==========================================
  Files          42       42              
  Lines        3496     3496              
==========================================
+ Hits         2630     2631       +1     
+ Misses        866      865       -1     

Files with missing lines	Coverage Δ
cognite/extractorutils/__init__.py	100.00% <100.00%> (ø)
cognite/extractorutils/configtools/loaders.py	83.85% <100.00%> (+0.07%)	⬆️
cognite/extractorutils/base.py	49.73% <20.00%> (+0.26%)	⬆️

[mathialo]

This is a good change, but it is going to result in a breaking change in every extractor using the utils, so we need to be a bit careful when releasing this.

[nithinb]

This is a good change, but it is going to result in a breaking change in every extractor using the utils, so we need to be a bit careful when releasing this.

@mathialo Yep I fully agree with this. I will include a version bump with valid release notes for the same. If the changes to address this look good then I will make the version changes as well.

[nithinb]

@eighty20results I was hoping you could take a look at the security changelog.
If it looks good I will reuse it with extractor PRs, to make the release

[eighty20results]

See comments

[nithinb]

@eighty20results the changes you requested have been made.
Once this goes through, I will create the respective PRs for all the extractors and cut a release for them as well

[mathialo]

@nithinb Sorry, but with #276 merged, there's now some conflicts with this PR. If you update the version num I think that will trigger a re-review. But just add me and I'll try to do it as fast as I can

[nithinb]

@mathialo no issues at all. I have updated the version. If this looks good, we can get the maturity-review/risk-review also involved.

[larshagencognite]

Is it feasible to add testing for this?

[mathialo]

@larshagencognite

Is it feasible to add testing for this?

In general, anything happening in the startup phase of extractors like this is really hard to unit test, for many reasons such as

• Parts of the startup is reading command line args and reacting to those. To test that would require a lot of monkeypatching.
• Libraries like our SDK, but especially prometheus, keeps state under the hood hidden from users, making it very hard to call them several times independently, such as in different tests. For example, if you define a Gauge with the same name twice - as would happen if you called the extractor startup twice - the second call would fail.

With the new rewrite of the utils for next-gen extractors, we are working to decompose some parts of the extractor runtime to make single parts easier to test, but there will still be areas that are just not really possible to unit test without so much monkeypatching and mocking that the test itself becomes meaningless.

For changes like this we are much more dependent on manual testing and thorough reviews.