Skip to content
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

EP10 config #1505

Merged
merged 56 commits into from Dec 6, 2023
Merged

EP10 config #1505

merged 56 commits into from Dec 6, 2023

Conversation

SpacemanPaul
Copy link
Contributor

@SpacemanPaul SpacemanPaul commented Oct 24, 2023

Reason for this pull request

Ground-up backwards-incompatible rewrite of config engine, as per ODC-EP10 Replace Configuration Layer

Proposed changes

  • New Config engine implemented and tested, as per EP.

  • Remove old config engine.

  • API documentation (in docstrings)

  • User documentation (for readthedocs)

  • Tests added / passed

  • Fully documented, including docs/about/whats_new.rst for all changes

@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (41c33cf) 85.19% compared to head (580c6c6) 85.37%.

Files Patch % Lines
datacube/scripts/system.py 59.25% 11 Missing ⚠️
datacube/cfg/opt.py 95.79% 5 Missing ⚠️
datacube/ui/click.py 77.27% 5 Missing ⚠️
datacube/cfg/api.py 97.34% 3 Missing ⚠️
datacube/drivers/postgis/_connections.py 88.88% 1 Missing ⚠️
datacube/drivers/postgres/_connections.py 90.90% 1 Missing ⚠️
datacube/index/_api.py 87.50% 1 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           develop-1.9    #1505      +/-   ##
===============================================
+ Coverage        85.19%   85.37%   +0.18%     
===============================================
  Files              131      137       +6     
  Lines            14551    14784     +233     
===============================================
+ Hits             12396    12622     +226     
- Misses            2155     2162       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

datacube/cfg/api.py Outdated Show resolved Hide resolved
datacube/cfg/api.py Outdated Show resolved Hide resolved
datacube/cfg/api.py Outdated Show resolved Hide resolved
Copy link
Member

@whatnick whatnick left a comment

Choose a reason for hiding this comment

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

Looks like a decent config update should merge and let errors in deployments pop out.

Copy link
Member

@omad omad left a comment

Choose a reason for hiding this comment

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

This all looks generally fine to me, thank you Paul! Sorry it's taken me so long to respond.

The documentation is very comprehensive. I think there's room for improvement it making it easy to understand, but that's not at all a blocker for merging.

I made a bunch of documentation fixes already, but there's a few more sphinx niceties I'll add, remove the need for the numbered headings.

Overall, my remaining concern is that this is still more complicated than we need, in terms of how configuration is resolved between searching for files in multiple places, as well as environment variables. But it is at least a bit simpler to think about than before, and the implementation looks a lot better.

datacube/cfg/api.py Show resolved Hide resolved
datacube/ui/click.py Show resolved Hide resolved
datacube/ui/click.py Show resolved Hide resolved
@SpacemanPaul SpacemanPaul merged commit 5be0e6e into develop-1.9 Dec 6, 2023
28 checks passed
@SpacemanPaul SpacemanPaul deleted the ep10-config branch December 6, 2023 01:22
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.

None yet

5 participants