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

Add datadog_monitor option to override the config file name #903

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

brentm5
Copy link
Contributor

@brentm5 brentm5 commented Sep 7, 2023

What does this PR do?

Allows users consuming the datadog_monitor resource to select a config file name instead of just the default of conf.yaml. The default still exists but by adding this option others can call the datadog_monitor function multiple times and not override existing files.

Motivation

This allows people to use the integration multiple times and place different config files in the same integration directory. The use case would be many different cookbooks adding a single check they care about without doing messy attribute overrides. The use case would be the following, Cookbook A & Cookbook B are both maintained by different teams but can be used on the same host. In the previous way of doing this you would have to add attributes in different cookbooks which can cause weird issues around attribute inheritance / ordering. By adding this teams can more easily just create different config files that are owned by the respective cookbooks and do not step on each other.

Cookbook A:

# Cookbook A

datadog_monitor 'process' do
  action :add
   <Existing configuration for the monitor>
  config_name 'datadog-agent'
end

# Cookbook B

datadog_monitor 'process' do
  action :add
   <Existing configuration for the monitor>
  config_name 'ssh'
end

Additional Notes

Possible Drawbacks / Trade-offs

I don't see too many Drawbacks, by default the existing behavior exists. The only drawback I can think of is this does add some complexity since there can be multiple files to manage however if you manage the with this resource then the delete function has the same logic.

Describe how to test/QA your changes

I am not entire sure what other tests should be added which is why I made this a Draft. The testing should be light, essentially create an integration (logs might be the easiest since it doesn't require an integration to run) and just validate that the new file is generated. The default case should already be tested.

Reviewer's Checklist

  • Add additional tests to validate the new behavior.

@brentm5 brentm5 force-pushed the bm-add-custom-config-file-option branch from f703c08 to 6ce901c Compare September 7, 2023 04:00
@brentm5 brentm5 marked this pull request as ready for review September 8, 2023 00:40
@brentm5 brentm5 requested a review from a team as a code owner September 8, 2023 00:40
@brentm5 brentm5 changed the title [WIP] Add datadog_monitor option to override the config file name Add datadog_monitor option to override the config file name Sep 8, 2023
@brentm5
Copy link
Contributor Author

brentm5 commented Oct 9, 2023

What are the steps to get a review or have a further discussion around the changes above?

@spencergilbert
Copy link

Sorry @brentm5! This fell through the cracks, I've added this to our next sprint so we should be taking a look in the next 2-3 weeks.

Copy link
Contributor

@alopezz alopezz left a comment

Choose a reason for hiding this comment

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

Thanks and apologies again for taking so long to look at this. This looks good implementation-wise, but it needs to be documented as otherwise customers may have trouble finding the feature.

I'm thinking something along the lines of adding references to it under the "Syntax" and "Properties" in https://github.com/DataDog/chef-datadog#integrations-without-recipes.

@brentm5 brentm5 force-pushed the bm-add-custom-config-file-option branch from 6ce901c to 31835a6 Compare August 9, 2024 15:24
@brentm5 brentm5 requested review from a team as code owners August 9, 2024 15:24
Copy link

@jhgilbert jhgilbert left a comment

Choose a reason for hiding this comment

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

Approved with a very minor suggestion, thanks!

README.md Outdated
| `use_integration_template` | Set to `true` (recommended) to use the default template, which writes the values of `instances`, `init_config`, and `logs` in the YAML under their respective keys. This defaults to `false` for backward compatibility, but may default to `true` in a future major version of the cookbook. |
| `config_name` | The filename used when creating an integrations configuration file. Overriding this property allows the creation of multiple configuration files for a single integration. This defaults to `conf` which creates a configuration file named `conf.yaml`. |

Choose a reason for hiding this comment

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

Suggested change
| `config_name` | The filename used when creating an integrations configuration file. Overriding this property allows the creation of multiple configuration files for a single integration. This defaults to `conf` which creates a configuration file named `conf.yaml`. |
| `config_name` | The filename used when creating an integrations configuration file. Overriding this property allows the creation of multiple configuration files for a single integration. This defaults to `conf`, which creates a configuration file named `conf.yaml`. |

This allows people to use the integration multiple times and place
different config files in the same integration directory.  The use case
would be many different cookbooks adding a single check they care about
without doing messy attribute overrides.
@brentm5 brentm5 force-pushed the bm-add-custom-config-file-option branch from 31835a6 to c5b1c1e Compare August 11, 2024 02:15
Copy link
Contributor

@alopezz alopezz left a comment

Choose a reason for hiding this comment

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

Cool, thanks!

@alopezz alopezz merged commit e27fcc6 into DataDog:main Aug 12, 2024
13 checks passed
@alopezz alopezz mentioned this pull request Aug 12, 2024
@brentm5 brentm5 deleted the bm-add-custom-config-file-option branch August 12, 2024 12:46
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.

4 participants