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
Add a config endpoint on a new api server #21025
Add a config endpoint on a new api server #21025
Changes from 13 commits
42b8aa8
02344fd
1e315df
66fe2eb
bcf2bcf
17b1c34
cbfadde
a1125dd
7f69bc9
59cd5c5
e457dc9
44113ca
69606a1
4b62f52
99993c7
d1c14cd
c21c66d
79ba640
23f8c5b
3a04b99
6d1e442
9ecb46c
fa3bfa2
ed704ae
6071290
27b9332
b7ea9f8
a6abb74
70fd44c
36685a1
72d395d
d6763cf
fde8155
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.
Do we have any tests around the token validation on either the old server code or the new?
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.
Not that I know of, I'll open a separate PR to fix that (I would like to merge this one as it's blocking other PRs)
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.
It may not be a convention in Go, but I see having too many parameters/arguments in a function as a code smell.
Maybe a suggestion for the CMD server could be its struct that we initialize and later call
startServer
or something. I also understand it might not be the right suggestion for this particular use case. At least we can start a conversation 😄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 definitely agree, IMO the API should at some point become a generic components others could register endpoints into, but this would be part of an eventual refactor of the API component
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.
FWIW, all these arguments will be removed in the near future as we'll embed them in the API Component directly (they will be injected to the API component through Fx)
See comment here:
datadog-agent/comp/api/api/component.go
Line 32 in b1ec829
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.
Do you need to call
StopServer
whenstartIPCServer
fail? Ideally whenstartIPCServer
returns an error,startIPCServer
should release any resources allocated if any.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.
Also, the fact that we are using global variables
cmdListener
andipcConfigListener
makes it more difficult to grok what is going on. Have you consider moving away from global variables and having server struct that holdscmdListener
andipcConfigListener
information? That way, we could have functions likeStopCMDServer
andStopIPCServer
, the same forstartCMDServer
andstartIPCServer
defined as part of the server structThere 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 know it was previously using
var listener net.Listener
but it might be a good opportunity to refactor such pattern 😄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.
@ogaca-dd I need to call
StopServer
ifstartIPCServer
fails because the other server was started beforehand, and needs to be stopped. I added some comments to make that clearer.@GustavoCaso This would be implemented in the comp PR, in this one I try to avoid changing things I don't need to
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.
You mean the API component PR?
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.
Yes, the componentization of the API will likely remove these global variables