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

Changed usage of ApplicationStartInfo to BinaryInfo #3045

Closed

Conversation

dhruv-vora
Copy link
Contributor

Description:

  • Renamed ApplicationStartInfo to BinaryInfo
  • Renamed ExeName to Command
  • Added Description to store the full name of the collector

@dhruv-vora
Copy link
Contributor Author

/cc @bogdandrutu @anuraaga

// passed into each component. This information can be overridden in custom builds.
type ApplicationStartInfo struct {
type BinaryInfo struct {
Copy link
Member

Choose a reason for hiding this comment

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

@Aneurysm9 what do you think about "BuildInfo" to be more in sync with golang debug.BuildInfo? Brainstorming so any ideas are welcome. Do we want to actually use the debug.BuildInfo.Path instead of description? That is something that we can get programmatically so may be better than Description?

Copy link
Member

Choose a reason for hiding this comment

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

BuildInfo sounds good to me. I don't think that debug.BuldInfo.Path works for our present use case, but I can definitely see where having that identifier that uniquely identifies the package and can be programmatically inserted into the BuildInfo is useful. Someone may copypasta the BuildInfo definition from this repo and not update the description, but they wouldn't be able to avoid having the toolchain report the package path it was built from. I'll work with @dhruv-vora to get a follow-up issue/PR created for that.

@bogdandrutu
Copy link
Member

Please rebase

@dhruv-vora
Copy link
Contributor Author

I have filed 2 separate PRs (#3056 and #3057) to split these changes which would supersede this PR. So closing this.

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.

3 participants