Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5994a30140
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return | ||
|
|
||
| # Overwrite the existing file | ||
| ddgl_config_path.write_text( |
There was a problem hiding this comment.
Does it make sense to have something that by default put ddtool related things in the config? That's an internal thing and would make it probably not working for any external contributor using it
There was a problem hiding this comment.
Sure, it's not ideal, but what would you suggest otherwise ? In order to make this easily usable by our internal contributors we need to put this "default config" somewhere, and it's always going to be opensource whether it's here or in datadog-agent :/
There was a problem hiding this comment.
External contributors wouldn't have access to our gitlabCI anyway, so there is no way to make this work for them at all (in terms of developing on the Agent). If they want to use ddgl with other projects they probably wouldn't use dda to install it anyway, and use the instructions on ddgl's repo itself (uv tool install ddgl)
Does the simple mention of ddtool existing pose a security risk ? Because in that case it's already in the docs 😅
There was a problem hiding this comment.
Yeah I do not think mentioning ddtool is an issue, and as you said it is already mentioned in a loooot of places.
I guess we can live with it. But what would be the impact of someone that cannot have ddtool installed running that command?
Not a blocker I think, this is mostly (only) used by us anyway
There was a problem hiding this comment.
If they were to use dda tools ddgl setup, it would then fail on the first invocation of ddgl saying something about an invalid token or command not found.
That gives me an idea: I can just add a check for if ddtool is on PATH and immediately exit if not. I'll add that 👍
This branch has not been deployed
2 participants
Addressing some feedback from ambassadors that
ddglwas a little bit hard to install and configure properly :)