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

Add public symbols checker script #1816

Merged
merged 30 commits into from
May 25, 2021
Merged

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented May 5, 2021

Description

This adds a tool that can be called with tox -e public-symbols-check and will show the user what public symbols are being added by a branch (as compared against main). For example, for this branch, the tool produces this output:

141 ocelotl@harrison:~/github/ocelotl/opentelemetry-python$ tox -e public-symbols-check 
public-symbols-check recreate: /home/ocelotl/github/ocelotl/opentelemetry-python/.tox/public-symbols-check
public-symbols-check installdeps: GitPython
public-symbols-check installed: gitdb==4.0.7,GitPython==3.1.14,smmap==4.0.0
public-symbols-check run-test-pre: PYTHONHASHSEED='1504796717'
public-symbols-check run-test: commands[0] | python /home/ocelotl/github/ocelotl/opentelemetry-python/scripts/public_symbols_checker.py
The test_public_symbols_check branch adds the following public symbols:

- opentelemetry-api/src/opentelemetry/trace/propagation/trucecontext.py
        TraceContextTextMapPropagator

- opentelemetry-api/src/opentelemetry/trace/span.py
        var

Please make sure that all of them are strictly necessary, if not, please consider prefixing them with an underscore to make them private.
___________________________________________________________________________________________ summary ____________________________________________________________________________________________
  public-symbols-check: commands succeeded
  congratulations :)

Fixes #1815

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test A

Does This PR Require a Contrib Repo Change?

Answer the following question based on these examples of changes that would require a Contrib Repo Change:

  • The OTel specification has changed which prompted this PR to update the method interfaces of opentelemetry-api/ or opentelemetry-sdk/

  • The method interfaces of test/util have changed

  • Scripts in scripts/ that were copied over to the Contrib repo have changed

  • Configuration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in

    • pyproject.toml
    • isort.cfg
    • .flake8
  • When a new .github/CODEOWNER is added

  • Major changes to project information, such as in:

    • README.md
    • CONTRIBUTING.md
  • Yes. - Link to PR:

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@ocelotl ocelotl added the Skip Changelog PRs that do not require a CHANGELOG.md entry label May 5, 2021
@ocelotl ocelotl requested a review from a team May 5, 2021 03:23
@ocelotl ocelotl self-assigned this May 5, 2021
@ocelotl ocelotl requested review from owais and lzchen and removed request for a team May 5, 2021 03:23
@ocelotl ocelotl force-pushed the issue_1815 branch 2 times, most recently from 77f52ee to d0d873d Compare May 5, 2021 04:07
@owais
Copy link
Contributor

owais commented May 5, 2021

This is awesome. It'd be even more amazing if we had a CI script that posted the output from this command as a comment to the PR. That'd be super useful.

@ocelotl
Copy link
Contributor Author

ocelotl commented May 5, 2021

This is awesome. It'd be even more amazing if we had a CI script that posted the output from this command as a comment to the PR. That'd be super useful.

Hm, good idea, let me see if I can implement it 💪

@ocelotl ocelotl force-pushed the issue_1815 branch 12 times, most recently from a1ca414 to 54135b0 Compare May 7, 2021 05:50
.github/workflows/test.yml Outdated Show resolved Hide resolved
@ocelotl ocelotl marked this pull request as draft May 12, 2021 22:51
@codeboten
Copy link
Contributor

@ocelotl please take a look at the lint failures.

@ocelotl
Copy link
Contributor Author

ocelotl commented May 25, 2021

Checking, @codeboten...

@ocelotl ocelotl marked this pull request as draft May 25, 2021 19:48
@ocelotl
Copy link
Contributor Author

ocelotl commented May 25, 2021

I am marking this PR as draft while I fix @aabmass comments ✌️

@ocelotl ocelotl marked this pull request as ready for review May 25, 2021 19:58
@ocelotl
Copy link
Contributor Author

ocelotl commented May 25, 2021

Comments have been addressed. I understand that new features can be added to make this checker better and smarter, but I suggest we start with this now. This way we at least have some protection for our public API. ✌️

Thank you all for your comments and ideas 👍

@codeboten codeboten merged commit 1470a8c into open-telemetry:main May 25, 2021
@ocelotl ocelotl deleted the issue_1815 branch June 1, 2021 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add tool to warn about public symbols being added
5 participants