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

Allow setting Headers in DownloadableFile message #197

Merged

Conversation

tpaschalis
Copy link
Member

@tpaschalis tpaschalis commented Jul 9, 2024

Fixes #194

This PR amends the spec and protos to include an optional set of headers in DownloadableFile, to be used in the download GET request to set up auth parameters.

This is only one of the ways we could implement setting authentication paramaters to be used by Agents when downloading packages, but it is consistent with other uses of the headers field and generic enough to cover different use cases so I feel it's worth considering.

Verified

This commit was signed with the committer’s verified signature.
virajjasani Viraj Jasani
Signed-off-by: Paschalis Tsilias <paschalis.tsilias@grafana.com>
@tpaschalis tpaschalis marked this pull request as ready for review July 9, 2024 15:28
@tpaschalis tpaschalis requested a review from a team July 9, 2024 15:28
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

Thank you @tpaschalis

LGTM. Once merged let's add corresponding implementation in OpAMP Go.

@andykellr I will keep this open for a couple days in case you have any comments.

@tpaschalis
Copy link
Member Author

Once merged let's add corresponding implementation in OpAMP Go.

Yes, I can handle that.

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

One thing I missed is that we want all new additions to start in [Development] status.

proto/opamp.proto Show resolved Hide resolved
@@ -2343,7 +2344,9 @@ should download the file:
and the file SHOULD be downloaded from the location specified in the
[download_url](#downloadablefiledownload_url) field of the
[DownloadableFile](#downloadablefile-message) message. The Agent SHOULD use an
HTTP GET message to download the file.
HTTP GET message to download the file. The Agent SHOULD include the HTTP
Copy link
Member

Choose a reason for hiding this comment

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

Let's mark this addition as [Development]. See how it's done in other places.

Copy link
Member Author

Choose a reason for hiding this comment

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

Apologies for missing that 🤦

Done, both in the proto definition, and in specification (both inline to the message and in the separate section).

Signed-off-by: Paschalis Tsilias <paschalis.tsilias@grafana.com>
@tigrannajaryan tigrannajaryan merged commit 86e8cf4 into open-telemetry:main Jul 30, 2024
5 checks passed
@tigrannajaryan
Copy link
Member

@tpaschalis thank you for adding this feature.

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.

Allow setting authentication in DownloadableFile message
2 participants