Skip to content

Add 'site' option support#464

Merged
truthbk merged 4 commits intomasterfrom
remicalixte/site-option
Nov 28, 2018
Merged

Add 'site' option support#464
truthbk merged 4 commits intomasterfrom
remicalixte/site-option

Conversation

@remicalixte
Copy link
Copy Markdown
Contributor

@remicalixte remicalixte commented Oct 22, 2018

No description provided.

truthbk
truthbk reviewed Oct 23, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@truthbk truthbk left a comment

Choose a reason for hiding this comment

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

Mostly looks good. Could you please add spec tests for the default + overriden cases?

Comment thread manifests/init.pp Outdated
@remicalixte
Copy link
Copy Markdown
Contributor Author

remicalixte commented Oct 23, 2018

I will add some tests

@olivielpeau
Copy link
Copy Markdown
Member

olivielpeau commented Oct 23, 2018

just a note: in datadog.yaml, dd_url needs to be unset for site to take effect. So, ideally, the module wouldn't write dd_url to datadog.yaml unless it's explicitly set by the user.

@remicalixte
Copy link
Copy Markdown
Contributor Author

remicalixte commented Oct 23, 2018 •
edited
Loading

dd_url is now empty by default but I had to add the default value for agent5 because it doesn't start with an empty dd_url

@remicalixte remicalixte force-pushed the remicalixte/site-option branch 12 times, most recently from 78eac75 to dec93c6 Compare October 23, 2018 21:51
@remicalixte remicalixte mentioned this pull request Oct 24, 2018
Merged
truthbk
truthbk suggested changes Nov 28, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@truthbk truthbk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

If I'm not mistaken we need to address the empty dd_url issue on older agent6 installs.

Comment thread manifests/init.pp Outdated
Copy link
Copy Markdown
Member

@truthbk truthbk Nov 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think the current manifest by default will yield a valid configuration for older A6 agents, unfortunately we need to support them: the problem we have here is that if this puppet manifest is used to install an older Agent6 (pre-site option) say 6.2.x it will yield a configuration with an empty dd_url and site set. It causes no harm to have site set, but the empty dd_url would be a no-no.

Copy link
Copy Markdown
Contributor Author

@remicalixte remicalixte Nov 28, 2018

Choose a reason for hiding this comment

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

Unlike A5, A6 has a default value for dd_url: https://github.com/DataDog/datadog-agent/blob/60fc833599c5015c15c741649840dc6bda930275/pkg/config/config.go#L75. That's why a left this parameter empty

Copy link
Copy Markdown
Contributor Author

@remicalixte remicalixte Nov 28, 2018

Choose a reason for hiding this comment

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

Just tested, with an empty dd_url, agent 6.2.0 still reports to app.datadoghq.com

Copy link
Copy Markdown
Member

@truthbk truthbk Nov 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This is accurate. Older agents DID specify a default for dd_url, apparently an empty string will not override this default vcalue. This was changed with the introduction of site, but this already accounts for that. 👍

@remicalixte remicalixte force-pushed the remicalixte/site-option branch from 73a5cdb to 14b2523 Compare November 28, 2018 19:21
truthbk
truthbk approved these changes Nov 28, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@truthbk truthbk left a comment

Choose a reason for hiding this comment

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

Nice work! Thank you! 🙇

@truthbk truthbk merged commit d45e121 into master Nov 28, 2018
@truthbk truthbk deleted the remicalixte/site-option branch November 28, 2018 20:35
@truthbk truthbk added this to the 2.4.0 milestone Dec 20, 2018
cegeka-jenkins pushed a commit to cegeka/puppet-datadog_agent that referenced this pull request Apr 6, 2020
@remicalixte @truthbk
Add 'site' option support (DataDog#464)
02f8a0f 
* Add 'site' option support

* Move default site to params

* Ensure dd_url is empty by default for agent 6

* Add spec test for new site option
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@truthbk truthbk truthbk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.4.0

Development

Successfully merging this pull request may close these issues.

3 participants

@remicalixte @olivielpeau @truthbk
⚡