Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: add env vars for proxy configurations #1120
base: next
Are you sure you want to change the base?
feat: add env vars for proxy configurations #1120
Changes from 1 commit
f5e701e
1f373db
b2379f1
0d5a621
0d4ea4f
ecbf0f5
ba5bace
38befc7
c4fb6d2
795b212
e4fe076
d4ff777
9732563
3697ff7
b169aff
ba17e63
816bdfc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we only use
clap
then I would suggest either directly copying--help
output here, or only documenting the important ones and tell the user to check out more options with--help
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have this as command options, but are used to create the
ProxyConfig
struct. ATM, I;ve removed the TOML file and we are trying to create theProxyConfig
struct with the env vars or default values if not present. But since we don't have it as options,--help
is not printing them.Though I can change it to be part of the command if we consider that is it more useful that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo the default should always be
cli
unless there is a compelling reason not to. I would make these allclap
variables with these default values, and with the env args set.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would basically mean that we merge
StartProxy
command andProxyConfig
structs, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct yes, at least from the external API sense.
One might still keep the
ProxyConfig
struct to decouple the cli interface from the proxy code but whether or not that's applicable depends.A good illustration of this ^^ is the node where currently each component has its own config, as well as a "normalized" variant for
miden-node start node
. When we refactor that we'll keep component config structs (but without toml support). Then its up to the clap/main binary code to convert between the cli args and the component config as it sees fit. e.g.This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should the text here be: "This command will make a request to the proxy to add the specified workers."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should the text here be: "This command will make a request to the proxy to remove the specified workers."