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

Agent Config Base Parsing #2665

Merged
merged 14 commits into from
Aug 18, 2023
Merged

Conversation

mattiekat
Copy link
Contributor

@mattiekat mattiekat commented Aug 16, 2023

Description

This is a big one for #2215...

  1. Splits the parsing of configs from the config definitions to support both the old, new, and future parsers simultaneously. (We might want to rename it from parser to config format or something; not set on this name).
  2. Implements the new config format without switching to it or enabling it. This means this PR does introduce dead code that is going to be used in the next PR.

Drive-by changes

  • Deleted some unused (or single use) macros defined in hyperlane base
  • Updated the definition of the config parsing trait to simplify implementation
  • Added some more ergonomic methods of handling errors in config parsing
  • Removed any raw unwrap() I noticed since these cause difficult to debug errors in prod.
  • Added some additional validation to catch code mistakes in config path definitions

Related issues

Backward compatibility

Yes

Testing

Manual (E2E only for now to make sure I did not break the old config format)

@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (7c989e7) 64.61% compared to head (ccf13a0) 64.61%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2665   +/-   ##
=======================================
  Coverage   64.61%   64.61%           
=======================================
  Files          90       90           
  Lines        1379     1379           
  Branches      182      182           
=======================================
  Hits          891      891           
  Misses        481      481           
  Partials        7        7           

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

@mattiekat mattiekat marked this pull request as ready for review August 16, 2023 20:46
Copy link
Contributor

@daniel-savu daniel-savu left a comment

Choose a reason for hiding this comment

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

LGTM

@mattiekat mattiekat enabled auto-merge (squash) August 18, 2023 19:30
@mattiekat mattiekat merged commit 389029d into main Aug 18, 2023
16 of 26 checks passed
@mattiekat mattiekat deleted the mattie/config/hyperlane-base-parsing branch August 18, 2023 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants