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

Updates beacon agent on-prem DSN examples #6213

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

ksnavely
Copy link
Contributor

@ksnavely ksnavely commented Nov 5, 2024

What Changed?

These changes are aligned with upcoming product capabilities.

I've also eliminated trailing whitespaces in the touched docs.

@ksnavely ksnavely requested a review from a team as a code owner November 5, 2024 20:14
@ksnavely ksnavely changed the base branch from develop to main November 5, 2024 20:31
@ksnavely ksnavely changed the base branch from main to develop November 5, 2024 20:32
@ksnavely ksnavely force-pushed the UPM-40771-agent-docs branch 3 times, most recently from 932d3e4 to 6a7d76b Compare November 5, 2024 20:37
@@ -158,8 +158,7 @@ agent:
provider:
onprem:
databases:
sales_reporting:
dsn: "$DSN"
- dsn: "$DSN"
Copy link
Contributor

Choose a reason for hiding this comment

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

In https://github.com/EnterpriseDB/docs/pull/6105/files a resource_id key is used; are we skipping it here for compactness?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resource id is an allowed field according to the code https://github.com/EnterpriseDB/upm-beacon/blob/79f12896839dd5c8898c97c4ef2591645de57a99/internal/config/agent.go#L211

It is an optional field though, and doesn't need to be specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, yes but it also conflicts with your edits which is what I want to resolve. Given the edits in here, I'd like to promote this PR and ensure nothing is lost from the other PR before we close it.

tags:
- "<tag-one>"
- "<tag-two>"
- dsn: "$DSN1"
Copy link
Contributor

Choose a reason for hiding this comment

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

As above - we have another PR where resource_id is used. The other option is that resource_id has been dropped, so in that case we should have a formal reference for the yaml in here.

Copy link
Contributor

@djw-m djw-m left a comment

Choose a reason for hiding this comment

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

As this clashes with another PR, some concerns over what is definitive. Probably best resolved with a YAML format listing for completeness.

@ksnavely
Copy link
Contributor Author

ksnavely commented Nov 6, 2024

I think this PR can be closed based on our conversation.

@djw-m
Copy link
Contributor

djw-m commented Nov 6, 2024

Happy to review and merge this one (as it has other fixes). We can doc resource_id properly elsewhere.

@djw-m djw-m self-requested a review November 7, 2024 09:46
Copy link
Contributor

@djw-m djw-m left a comment

Choose a reason for hiding this comment

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

LGTM - will hold till it is confirmed released.

@djw-m
Copy link
Contributor

djw-m commented Nov 7, 2024

Do not merge until @ksnavely or @sonotley confirm that this has been released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants