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

op-deployer: add intent-config-type #12970

Merged
merged 14 commits into from
Nov 27, 2024
Merged

op-deployer: add intent-config-type #12970

merged 14 commits into from
Nov 27, 2024

Conversation

bitwiseguy
Copy link
Contributor

@bitwiseguy bitwiseguy commented Nov 19, 2024

Creates the concept of intent-config-type. Based on the selection, some fields are auto-populated and others are expected to be populated by the user prior to running op-deployer apply. Valid options are:

  • test used by current test suite, deterministically generates addresses/keys used in testing
  • standard (default, uses standard values as defined in superchain-registry where possible)
  • custom requires all params to be populated by user, errors during apply if any don't get set
  • strict same as "standard", but sets additional params
  • standard-overrides
  • strict-overrides

Design Details

  • op-deployer init
    • intent.SetInitValues: generates initial intent (with many empty fields)
    • user expected to fill out empty/zero-value fields before running op-deployer apply
  • op-deployer apply
    • pipeline stage: init
      • intent.ValidateIntentConfigType: validates that all fields have been populated, and that values meet expectation (e.g. standard values are used if intent-config-type=standard

Open questions

Which fields should thestandard-overrides/strict-overrides settings allow to be overridden? The way this is currently implemented:

  • standard-overrides:
    • op-deployer init: sets intent.toml standard values
    • op-deployer apply: validates that all values are populated (i.e. non zero value) but doesn't require specific values
  • strict-overrides:
    • op-deployer init: sets intent.toml strict values
    • op-deployer apply: validates that all values are populated (i.e. non zero value) but doesn't require specific values

Meta

Closes https://github.com/ethereum-optimism/platforms-team/issues/305

@bitwiseguy bitwiseguy added the M-do-not-merge Meta: Do not merge label Nov 19, 2024
@bitwiseguy bitwiseguy force-pushed the ss/intent-config-type branch from a7d91eb to 9d0e556 Compare November 22, 2024 18:17
@bitwiseguy bitwiseguy changed the title op-deployer: add config-intent-type op-deployer: add intent-config-type Nov 22, 2024
@bitwiseguy bitwiseguy marked this pull request as ready for review November 22, 2024 21:16
@bitwiseguy bitwiseguy requested a review from a team as a code owner November 22, 2024 21:16
@bitwiseguy bitwiseguy requested review from axelKingsley, mslipper and mds1 and removed request for axelKingsley November 22, 2024 21:16
@bitwiseguy bitwiseguy removed the M-do-not-merge Meta: Do not merge label Nov 22, 2024
Copy link
Collaborator

@mslipper mslipper left a comment

Choose a reason for hiding this comment

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

A few issues I'd like to address before merge. Happy to pair next week to resolve them.

op-deployer/pkg/deployer/state/intent.go Outdated Show resolved Hide resolved
op-deployer/pkg/deployer/state/intent.go Outdated Show resolved Hide resolved
op-deployer/pkg/deployer/state/intent.go Outdated Show resolved Hide resolved
op-deployer/pkg/deployer/state/intent.go Outdated Show resolved Hide resolved
op-deployer/pkg/deployer/state/intent.go Outdated Show resolved Hide resolved
op-deployer/pkg/deployer/state/intent.go Outdated Show resolved Hide resolved
op-deployer/pkg/deployer/state/intent.go Outdated Show resolved Hide resolved
@mslipper mslipper enabled auto-merge November 27, 2024 16:10
@bitwiseguy bitwiseguy force-pushed the ss/intent-config-type branch from fc440e4 to 3af7850 Compare November 27, 2024 16:11
Copy link

codecov bot commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 124 lines in your changes missing coverage. Please review.

Project coverage is 42.81%. Comparing base (9109958) to head (55376b2).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
op-deployer/pkg/deployer/state/intent.go 49.15% 80 Missing and 10 partials ⚠️
op-deployer/pkg/deployer/standard/standard.go 32.00% 17 Missing ⚠️
op-deployer/pkg/deployer/init.go 0.00% 6 Missing ⚠️
op-deployer/pkg/deployer/state/chain_intent.go 82.35% 4 Missing and 2 partials ⚠️
op-deployer/pkg/deployer/apply.go 0.00% 2 Missing and 1 partial ⚠️
op-deployer/pkg/deployer/pipeline/init.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #12970      +/-   ##
===========================================
- Coverage    44.46%   42.81%   -1.66%     
===========================================
  Files          798      743      -55     
  Lines        71682    67175    -4507     
===========================================
- Hits         31876    28762    -3114     
+ Misses       37219    35990    -1229     
+ Partials      2587     2423     -164     
Flag Coverage Δ
cannon-go-tests-32 ?
cannon-go-tests-64 ?

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

Files with missing lines Coverage Δ
op-deployer/pkg/deployer/artifacts/locator.go 82.60% <100.00%> (+0.97%) ⬆️
op-deployer/pkg/deployer/flags.go 62.50% <ø> (ø)
op-deployer/pkg/deployer/pipeline/init.go 32.74% <0.00%> (ø)
op-deployer/pkg/deployer/apply.go 49.33% <0.00%> (-0.67%) ⬇️
op-deployer/pkg/deployer/init.go 0.00% <0.00%> (ø)
op-deployer/pkg/deployer/state/chain_intent.go 82.35% <82.35%> (ø)
op-deployer/pkg/deployer/standard/standard.go 49.59% <32.00%> (+0.09%) ⬆️
op-deployer/pkg/deployer/state/intent.go 46.69% <49.15%> (+46.69%) ⬆️

... and 66 files with indirect coverage changes

@mslipper mslipper added this pull request to the merge queue Nov 27, 2024
Merged via the queue into develop with commit c8f4b3a Nov 27, 2024
45 checks passed
@mslipper mslipper deleted the ss/intent-config-type branch November 27, 2024 19:50
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.

2 participants