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

Updating method definitions to reflect contribution guide changes #4865

Conversation

MovieStoreGuy
Copy link
Contributor

@MovieStoreGuy MovieStoreGuy commented Feb 15, 2022

Description:
Addresses how methods must be defined depending on their purpose according to the contribution guide.

Link to tracking Issue:
Closes #4471

Testing:
N/A

Documentation:
Already exists in the contribution guide.

I did have a thought of keeping the existing method but marking the exported method as deprecated to not immediately force changes inside the collector contrib on next release, not sure what is preferred here.

@MovieStoreGuy MovieStoreGuy requested a review from a team as a code owner February 15, 2022 03:27
@project-bot project-bot bot added this to In progress in Collector Feb 15, 2022
@dmitryax
Copy link
Member

It should be going through a deprecation process established recently for this repo https://github.com/open-telemetry/opentelemetry-collector/blob/main/CONTRIBUTING.md#breaking-changes

@codecov
Copy link

codecov bot commented Feb 15, 2022

Codecov Report

Merging #4865 (45cbac9) into main (2a2a722) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 45cbac9 differs from pull request most recent head 9b07d5f. Consider uploading reports for the commit 9b07d5f to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4865   +/-   ##
=======================================
  Coverage   90.64%   90.64%           
=======================================
  Files         180      180           
  Lines       10627    10627           
=======================================
  Hits         9633     9633           
  Misses        776      776           
  Partials      218      218           
Impacted Files Coverage Δ
cmd/builder/internal/builder/config.go 83.82% <100.00%> (ø)
config/confighttp/confighttp.go 100.00% <100.00%> (ø)
exporter/exporterhelper/common.go 94.25% <100.00%> (ø)
exporter/exporterhelper/queued_retry.go 96.35% <100.00%> (ø)
exporter/exporterhelper/queued_retry_inmemory.go 95.58% <100.00%> (ø)
exporter/otlpexporter/factory.go 91.54% <100.00%> (ø)
exporter/otlphttpexporter/factory.go 87.62% <100.00%> (ø)
internal/testcomponents/example_factories.go 79.54% <100.00%> (ø)
receiver/scraperhelper/scrapercontroller.go 93.93% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a2a722...9b07d5f. Read the comment docs.

@MovieStoreGuy
Copy link
Contributor Author

@dmitryax, I am not entirely keen on adding test coverage for the deprecated method wrappers since it would trip over the linter (gosec) though I am not sure if that is reasonable.

// DefaultHTTPClientSettings returns HTTPClientSettings type object with
// Deprecated: [v0.45.0] Use NewDefaultHTTPClientSettings instead.
func DefaultHTTPClientSettings() HTTPClientSettings {
return NewDefaultHTTPClientSettings()
Copy link
Member

Choose a reason for hiding this comment

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

Using alias will make codecov happy

var DefaultHTTPClientSettings = NewDefaultHTTPClientSettings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this works, mind if I update the recommendation inside the contribution guide?

Copy link
Member

Choose a reason for hiding this comment

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

Go for it in a separate PR :)

@MovieStoreGuy MovieStoreGuy force-pushed the smarciniak/update-method-signatures branch from ad2e9bf to 23012cc Compare February 16, 2022 05:42
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

sed/[v0.45.0]/[v0.46.0]
sed/[v0.44.0]/[v0.46.0]

Collector automation moved this from In progress to Reviewer approved Feb 17, 2022
@MovieStoreGuy MovieStoreGuy force-pushed the smarciniak/update-method-signatures branch from 23012cc to 9c8921e Compare February 17, 2022 00:54
@MovieStoreGuy
Copy link
Contributor Author

@bogdandrutu , rebased with main and updated the deprecated version numbers.

@bogdandrutu
Copy link
Member

A CHNAGELOG entry would make this PR ready to merge :)

@MovieStoreGuy MovieStoreGuy force-pushed the smarciniak/update-method-signatures branch 2 times, most recently from 0efdb09 to 24387eb Compare February 18, 2022 01:33
@MovieStoreGuy MovieStoreGuy force-pushed the smarciniak/update-method-signatures branch from 45cbac9 to 9b07d5f Compare February 18, 2022 01:39
@bogdandrutu bogdandrutu merged commit d5656d6 into open-telemetry:main Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Collector
  
Reviewer approved
Development

Successfully merging this pull request may close these issues.

Inconsistency when using DefaultFoo vs NewDefaultFoo
3 participants