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
Specify User Agent for Console CLI requests #952
Specify User Agent for Console CLI requests #952
Changes from all commits
67a196c
3fa9a89
cf1a5ba
1219506
712a256
662d28a
796d6b2
b488a73
352f23b
5c01347
a733a61
3cc59c0
c5c69dd
346ef9c
21724b9
9c8868c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Okay, I think I slightly regret YAML vs env variable decision because we could have kept env variable stuff much more limited to only grabbing it at the level of each client, instead of having to pass it all the way through the code -- it feels like conceptually this shouldn't have to be woven through all these factories and lots of unrelated code and it should just stay low level, but I don't really have any way in mind to implement that if it comes from the YAML.
Which is all to say, I'm not sure if there's a better solution or not, and definitely not sure whether it's worth refactoring this, because I think it's well-implemented for this route. Thoughts?
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.
(Circling back to add that after looking at the tests, this services.yaml approach is definitely clearer and easier to test, so that's a point in its favor)
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.
From some discussion with Mikayla there are pros and cons with the current approach versus setting something more globally instead of passing through yaml. I'm comfortable with the current approach for now, and think it can be improved so there are less touch points next time we want some global settings but let me know if you have concerns @mikaylathompson