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

[SDK] Simplify SDK version #2150

Closed
marcalff opened this issue May 23, 2023 · 2 comments · Fixed by #2180
Closed

[SDK] Simplify SDK version #2150

marcalff opened this issue May 23, 2023 · 2 comments · Fixed by #2180
Assignees
Labels
bug Something isn't working

Comments

@marcalff
Copy link
Member

Some fields exposed in sdk::version are not useful, and create unnecessary confusion.

Field sdk::version::branch

This is the name of a temporary branch used on a maintainer machine while preparing the build.

The branch name is not created in the opentelemetry-cpp repository itself, and does not allow someone to fork the repository and start changes from this "branch", as git tags are used instead.

This field serves no purpose, and is misleading, as the branch referred to can not be found anywhere.

Field sdk::version::commit_hash

This commit hash was likely intended to be the commit that created the release.

Because a commit can not reference itself, at best this commit hash is not the commit released to publish a version, but the previous commit, which introduce a discrepancy between the commit hash published in sdk::version and the commit hash used to create the version tag in github.

In addition, because merge policies are set to squash a PR when merging to main, the intermediate commits found in a PR branch are not preserved when merging to the main branch in the github repository.

This field serves no purpose, and is misleading, as the commit referred to can not be found in the github opentelemetry-cpp repository.

Field sdk::version::count_new_commits

This field was likely intended to record how many commits have been added since the last release.

This field is however unreliable:

  • when commits are merged with a squash, the commit count changes
  • should a fork of the opentelemetry-cpp repository produce a different release, the count_new_commit field is not guaranteed to be updated during the build (this is a manual process), so that a value seen at runtime in a given application for sdk::version::count_new_commits can not be trusted as accurate.

Ultimately, the git history in the github repository is the source of truth to inspect differences between two versions.

The purpose for sdk::version::count_new_commits is unclear, and the value found can be misleading.

How an application is expected to make use of this data is unclear.

Suggested fix

Remove entirely:

  • Field sdk::version::branch
  • Field sdk::version::commit_hash
  • Field sdk::version::count_new_commits

and simplify release scripts accordingly.

@marcalff marcalff added the bug Something isn't working label May 23, 2023
@marcalff marcalff self-assigned this May 23, 2023
@marcalff
Copy link
Member Author

To discuss, and fix after 1.9.1 is released, to avoid last minute changes.

@lalitb
Copy link
Member

lalitb commented May 24, 2023

Thanks for the issue, and all the details. The initial plan was to generate version.cc through automated release pipeline, which would ensure that all these fields would be correctly populated in the source release archive. The version.cc tracked in git would then always have these fields empty, or probably it won't exist. The pre_commit script was supposed to be a hook to be called during release. But somehow, the automation never kicked-in and we started adding these fields manually (or manually running pre_commit), and so some of the fields were not accurate.

In current setup, I don't see much relevance of these 3 suggested fields, and vote for their removal.

marcalff added a commit to marcalff/opentelemetry-cpp that referenced this issue Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants