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

remove need to clone core repo #678

Merged
merged 8 commits into from
Sep 27, 2021

Conversation

codeboten
Copy link
Contributor

Description

Companion PR to open-telemetry/opentelemetry-python#2108, this removes the need to clone the core repo in order to run tox. It uses pip instead.

NOTE: This change also removes lint ignore rules for the opentelemetry-python-core subdirectory. This means after this PR, contributors should remove that folder from their local environment.

@codeboten codeboten added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Sep 13, 2021
@codeboten codeboten requested a review from a team September 13, 2021 22:54
@owais
Copy link
Contributor

owais commented Sep 14, 2021

Generate for bootstrap needs to be updated. I guess we should move it to core and use something like this as source of truth for generate

I was planning to come back and finish the meta package PR next week but if it helps, feel free to take it over. #516

@codeboten
Copy link
Contributor Author

@owais yah that's the last thing i need to fix for this PR to be done. will take a look sometimes this week.

@lzchen
Copy link
Contributor

lzchen commented Sep 14, 2021

@owais
Which files for bootstrap needs to be updated exactly? (or not, if we are changing it)

@owais
Copy link
Contributor

owais commented Sep 14, 2021

@lzchen
Copy link
Contributor

lzchen commented Sep 14, 2021

@owais
The gen file has to be updated every release. Is that related to this PR though?

@lzchen
Copy link
Contributor

lzchen commented Sep 14, 2021

@owais
Oh I guess opentelemetry-python-core needs to be checked out to run eachdist.py for generating?

@owais
Copy link
Contributor

owais commented Sep 14, 2021

@codeboten see if this helps: #681

Gen script in core can install the eopentelemetry-instrumentation-all egg from contrib repo like other packages as a dev dependency and just copy over the __init__.py.

@owais
Copy link
Contributor

owais commented Sep 14, 2021

Oh I guess opentelemetry-python-core needs to be checked out to run eachdist.py for generating?

Right. That's how it works today but with the changes @codeboten is making, it shouldn't be necessary going forward.

@codeboten
Copy link
Contributor Author

@owais please take another look. I've updated generate_instrumentation_bootstrap.py to:

  • pull down bootstrap_gen.py from the core repo via requests and store it in a temp folder
  • generate the new bootstrap_gen.py and compare the two files
  • if they differ, the script returns an error, which will cause CI to fail, rather than running git diff as was done before

Let me know what you think.

@codeboten codeboten requested a review from owais September 14, 2021 21:19
@owais
Copy link
Contributor

owais commented Sep 14, 2021

IIUR, this will tell the author that bootstrap_gen in core needs an update and then the author will have to go and create a PR in core by manually copying/generating the file. Is that right?

@owais
Copy link
Contributor

owais commented Sep 14, 2021

It would be great if a bot could create PR in core repo every time the file changed in contrib main but I'm working on a proposal related to distros that might help avoid this completely.

@codeboten
Copy link
Contributor Author

IIUR, this will tell the author that bootstrap_gen in core needs an update and then the author will have to go and create a PR in core by manually copying/generating the file. Is that right?

@owais yup that's correct. it's not great, but it seems the benefits of removing the core repo from regular dev workflow outweighs the burden to manually copy this file when necessary. I like the idea of removing the need for this altogether though, that sounds promising! Thoughts?

@owais
Copy link
Contributor

owais commented Sep 15, 2021

Definitely! I wasn't against it. Just trying to verify if I understood the new flow properly. We should just document the workflow here so outside contributors know what they are supposed to do to fix the CI failure.

current_path = os.path.join(tmpdir.name, "current.py",)

core_repo = os.getenv("CORE_REPO_SHA", "main")
url = "https://raw.githubusercontent.com/open-telemetry/opentelemetry-python/{}/opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap_gen.py".format(
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to make this a f-string

@ocelotl
Copy link
Contributor

ocelotl commented Sep 27, 2021

@codeboten, please check lint.

@codeboten
Copy link
Contributor Author

@ocelotl fixed the lint errors

@lzchen lzchen enabled auto-merge (squash) September 27, 2021 20:01
@lzchen lzchen merged commit d2984f5 into open-telemetry:main Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants