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

logproto: Add deprecated annotation to LegacySample and LegacyLabelPair #5454

Merged
merged 2 commits into from
Feb 24, 2022

Conversation

tpaschalis
Copy link
Member

@tpaschalis tpaschalis commented Feb 23, 2022

Signed-off-by: Paschalis Tsilias paschalis.tsilias@grafana.com

What this PR does / why we need it:
The PR adds a docstring to those two messages, to discourage accidental use.

The changes were just
a) Adding a docstring on the LegacySample and LegacyLabelPair messages
b) Run make protos

Basically just familiarizing myself with the contributing process for Loki :)

Which issue(s) this PR fixes:
Fixes #5312

Special notes for your reviewer:
I initially tried adding an option like option deprecated = true; or a comment like // Deprecated: Use Sample instead, as I thought that this might translate better for other languages' proto compilers.

Unfortunately, both these approaches trip staticheck, which complains about using a deprecated struct in tests and code. From its source, it seems that it will flag all structs where there's a docstring line with the prefix Deprecated:.

So instead of littering the codebase with annotations such as //nolint:staticcheck // Ignore SA1019 for deprecated fields, I opted for a more generic annotation.

The drawback is that it wouldn't autogenerate language-specific deprecation notices for other proto compilers (like @Deprecated in Java). Would you be okay with that?

Also, I'm not sure if this warrants a change in the changelog or documentation (I'd say no), but let me know if you think otherwise.

Checklist

  • Documentation added (N/A)
  • Tests updated (N/A)
  • Add an entry in the CHANGELOG.md about the changes. (N/A)

Signed-off-by: Paschalis Tsilias <paschalis.tsilias@grafana.com>
@CLAassistant
Copy link

CLAassistant commented Feb 23, 2022

CLA assistant check
All committers have signed the CLA.

Signed-off-by: Paschalis Tsilias <paschalis.tsilias@grafana.com>
@tpaschalis tpaschalis marked this pull request as ready for review February 23, 2022 14:30
@tpaschalis tpaschalis requested a review from a team as a code owner February 23, 2022 14:30
@tpaschalis
Copy link
Member Author

cc @DylanGuedes

@DylanGuedes
Copy link
Contributor

Thanks! 🎉

Sad to hear that adding the deprecated annotation would cause so much trouble. Anyway, having this comment/annotation is better than not having it, so LGTM.

And yeah, I think this shouldn't be listed in the changelog, as it is a kinda internal change.

Copy link
Contributor

@kavirajk kavirajk left a comment

Choose a reason for hiding this comment

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

LGTM. thanks @tpaschalis

@kavirajk kavirajk merged commit cbd3b00 into grafana:main Feb 24, 2022
@tpaschalis tpaschalis deleted the logproto-deprecated-annotation branch February 24, 2022 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add deprecation notice to LegacySample and LegacyLabelPair
4 participants