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

Refactor bootstrap generation #2101

Merged
merged 6 commits into from
May 2, 2024
Merged

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Dec 21, 2023

This makes the bootstrap script get the package version directly from pypi instead of from our lists of packages. This makes sure that the packages are actually available for the end user to install.

Fixes #2053

@ocelotl ocelotl added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Dec 21, 2023
@ocelotl ocelotl self-assigned this Dec 21, 2023
@ocelotl ocelotl requested a review from a team December 21, 2023 03:51
@ocelotl ocelotl force-pushed the issue_2053 branch 2 times, most recently from b1885ea to 1683b7f Compare December 22, 2023 23:26
@lzchen
Copy link
Contributor

lzchen commented Dec 27, 2023

Just to clarify, in our release process, we change each package to the release version and then we regenerate bootstrap_gen.py. I'm curious as to how you generated the 0.43b0 versions of bootstrap_gen.py, did you run the new script or did you replace these manually? I think we just need to add a try catch for erroneous installs, there's no need to change what is generated in our dev branch right?

@lzchen
Copy link
Contributor

lzchen commented Dec 27, 2023

I think we allow users to specify version in bootstrap so probably have to still reflect this in the install.

@lzchen
Copy link
Contributor

lzchen commented Dec 27, 2023

I'm not sure if this is exactly a workaround for #2053 since users will still see an error message if the package doesn't exist in Pypi right? I think there isn't really a workaround since there's no way to provide the actual package (the most we can do is not display the error message and just explain on the issues that the package is not available yet).

@ocelotl
Copy link
Contributor Author

ocelotl commented Jan 2, 2024

Just to clarify, in our release process, we change each package to the release version and then we regenerate bootstrap_gen.py. I'm curious as to how you generated the 0.43b0 versions of bootstrap_gen.py, did you run the new script or did you replace these manually? I think we just need to add a try catch for erroneous installs, there's no need to change what is generated in our dev branch right?

Right, reverted to 44b0.dev.

@ocelotl
Copy link
Contributor Author

ocelotl commented Jan 2, 2024

Just to clarify, in our release process, we change each package to the release version and then we regenerate bootstrap_gen.py. I'm curious as to how you generated the 0.43b0 versions of bootstrap_gen.py, did you run the new script or did you replace these manually? I think we just need to add a try catch for erroneous installs, there's no need to change what is generated in our dev branch right?

Right, reverted to 44b0.dev.

Which fails tox -e generate...

@ocelotl
Copy link
Contributor Author

ocelotl commented Jan 2, 2024

I'm not sure if this is exactly a workaround for #2053 since users will still see an error message if the package doesn't exist in Pypi right? I think there isn't really a workaround since there's no way to provide the actual package (the most we can do is not display the error message and just explain on the issues that the package is not available yet).

This is a workaround because no exception is being raised now, only an error message is now produced for the particular package.

@aabmass
Copy link
Member

aabmass commented Feb 1, 2024

Fixes #2053

I think this fixes the bootstrap script issue, but doesn't actually solve the PyPI error you would when trying to install the non-existent release

@tammy-baylis-swi
Copy link
Contributor

I think this fixes the bootstrap script issue, but doesn't actually solve the PyPI error you would when trying to install the non-existent release

Seconding this, and was also mentioned at today's sig meeting. In this PR description I've included an example error if a user with aiohttp installed tries to bootstrap with the latest release: #2299. It would be great to have this part fixed.

Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

LGTM. @ocelotl please remove the "fixes" line in the pr description as this pr doesn't actually fully resolve the original issue.

@npuichigo
Copy link

any update on this?

This makes the bootstrap script get the package version directly from
pypi instead of from our lists of packages. This makes sure that the
packages are actually available for the end user to install.

Fixes open-telemetry#2053
@ocelotl ocelotl merged commit 1ee7261 into open-telemetry:main May 2, 2024
314 checks passed
ocelotl added a commit to ocelotl/opentelemetry-python-contrib that referenced this pull request May 10, 2024
ocelotl added a commit that referenced this pull request May 14, 2024
* Revert "Refactor bootstrap generation (#2101)"

This reverts commit 1ee7261.

* Add error handling to opentelemetry-bootstrap -a

Fixes #2516

---------

Co-authored-by: Tammy Baylis <[email protected]>
shadchin pushed a commit to shadchin/opentelemetry-python-contrib that referenced this pull request May 29, 2024
* Revert "Refactor bootstrap generation (open-telemetry#2101)"

This reverts commit 1ee7261.

* Add error handling to opentelemetry-bootstrap -a

Fixes open-telemetry#2516

---------

Co-authored-by: Tammy Baylis <[email protected]>
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.

ERROR: No matching distribution found for opentelemetry-instrumentation-aiohttp-server==0.42b0
5 participants