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

Configuration for new generator functionality. #674

Merged

Conversation

cody-littley
Copy link
Contributor

@cody-littley cody-littley commented Jul 30, 2024

Why are these changes needed?

This code was broken off of another PR that got too large: #666

This PR contains configuration for the new generator functionality. It is not utilized in this branch, but it will be utilized once the rest of the code merges.

Checks

  • I've made sure the lint is passing in this PR.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • Testing Strategy
    • Unit tests (in the other PR)
    • Integration tests
    • This PR is not tested :(

Signed-off-by: Cody Littley <cody@eigenlabs.org>
@cody-littley cody-littley self-assigned this Jul 30, 2024
envPrefix = "TRAFFIC_GENERATOR"
)

var (
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of listing flags for dependent packages again, we could import the flags directly like here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

change made

)

// Config configures a traffic generator.
type Config struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could reuse the configs from dependencies directly like we do here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

change made

Signed-off-by: Cody Littley <cody@eigenlabs.org>
TheGraphConfig *thegraph.Config

// Configuration for the EigenDA client.
EigenDaClientConfig *clients.EigenDAClientConfig
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: EigenDAClientConfig

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

In java, the prevailing style is to capitalize only the first letter of an acronym. I find the java pattern ugly, but muscle memory takes time to retrain. 🙃

@@ -0,0 +1,262 @@
package config
Copy link
Contributor

Choose a reason for hiding this comment

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

are these moved from /tools/traffic/flags.go? or are these being added in addition to the existing ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the short term, the new flags.go file will duplicate the original file in the master branch, but will not be used for anything. When the rest of the code from this branch merges, the new flags.go will entirely replace the old one.

I'm merging this way so that I can split off the config changes into a stand alone PR. It can't replace the old flags because it's not fully backwards compatible.

Signed-off-by: Cody Littley <cody@eigenlabs.org>
@cody-littley cody-littley merged commit c1592a3 into Layr-Labs:master Aug 2, 2024
6 checks passed
@cody-littley cody-littley deleted the generator-config-fragment branch August 15, 2024 13:25
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