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

Introduce Poetry and bump up e2e #249

Merged
merged 6 commits into from Nov 10, 2022
Merged

Conversation

Superskyyy
Copy link
Member

@Superskyyy Superskyyy commented Nov 7, 2022

A step in preparing the 1.0.0 release.

  • Replace venv/pip with Poetry
  • Bump up e2e dependencies (OAP, swctl)
  • Support slim python docker image
    Slim docker images are tested in CI and produced by the make build-image process.

The following changes are mostly side effects encountered during the migration.

  • Remove Python 3.6 from the testing matrix (removal of support will come in the next PR)
    Poetry and planned os.register_at_fork API usage in agent requires 3.7+, and 3.6 has reached end of support.

  • Update documentation to reflect poetry migration
    New docs on how to setup development env make env, make test etc.

  • Update PR template
    Reflect ^ above changes.

  • Bump up swctl & OAP version in e2e
    Bump up to 9.2.0

  • fix log list e2e, trace list e2e, service instance (.total, errorreason, layer concept)
    Reflect changes in expected results due to the bump up.

  • fix kafka ignore (parenthesis missing)
    Kafka plugin self data ignore

  • fix meter e2e (naming)
    Fix meter e2e thanks to the help from @jiang1997 (fix typo that invalids python-runtime.yaml skywalking#9912)

* Replace venv/pip with Poetry

* Update documentation to reflect poetry migration

* Update PR template

* Support slim python docker image

* Bump up swctl & OAP version in e2e

* fix kafka ignore (parenthesis missing)

* fix meter e2e (nameing)

* fix log list e2e, trace list e2e, service instance (.total, errorreason, layer concept)
@Superskyyy Superskyyy self-assigned this Nov 7, 2022
@Superskyyy Superskyyy added documentation Improvements or additions to documentation chore Project chores test test labels Nov 7, 2022
@Superskyyy Superskyyy added this to the 1.0.0 milestone Nov 7, 2022
@Superskyyy
Copy link
Member Author

@jiang1997 if you have time, please help to try the changes out (the poetry part), make env command etc. And see if they work in your environment.

@jiang1997
Copy link
Contributor

@jiang1997 if you have time, please help to try the changes out (the poetry part), make env command etc. And see if they work in your environment.

Got it.

@Superskyyy Superskyyy marked this pull request as ready for review November 7, 2022 16:21
@kezhenxu94
Copy link
Member

I cannot run this and failed to find a workable solution in Google, I did found some similar questions/issues but none works for me. Haven't dig deep to see why and how to fix this yet.

skywalking-python pr/Superskyyy/249 % make
curl -sSL https://install.python-poetry.org | python3 -
Retrieving Poetry metadata

# Welcome to Poetry!

This will download and install the latest version of Poetry,
a dependency and package manager for Python.

It will add the `poetry` command to Poetry's bin directory, located at:

/Users/kezhenxu94/.local/bin

You can uninstall at any time by executing this script with the --uninstall option,
and these changes will be reverted.

Installing Poetry (1.2.2): Creating environment
Traceback (most recent call last):
  File "<stdin>", line 940, in <module>
  File "<stdin>", line 919, in main
  File "<stdin>", line 550, in run
  File "<stdin>", line 571, in install
  File "/Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/contextlib.py", line 117, in __enter__
    return next(self.gen)
  File "<stdin>", line 643, in make_env
  File "<stdin>", line 629, in make_env
  File "<stdin>", line 309, in make
  File "/Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/venv/__init__.py", line 66, in __init__
    self.symlinks = should_use_symlinks(symlinks)
  File "/Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/venv/__init__.py", line 31, in should_use_symlinks
    raise Exception("This build of python cannot create venvs without using symlinks")
Exception: This build of python cannot create venvs without using symlinks
make: [poetry] Error 1 (ignored)
poetry self update
make: poetry: No such file or directory
make: *** [poetry] Error 1
skywalking-python pr/Superskyyy/249 % 

@Superskyyy
Copy link
Member Author

Superskyyy commented Nov 8, 2022

I cannot run this and failed to find a workable solution in Google, I did found some similar questions/issues but none works for me. Haven't dig deep to see why and how to fix this yet.

Seems like a macOS's built-in python (not for other python versions) issue here python-poetry/install.python-poetry.org#24

There isn't a final fix yet, but installing poetry from homebrew looks like an easy workaround for now. (Can you see if it works?)
If so we could do something like this until they fix it?

ifeq ($(OS),Windows_NT)
    OS := Windows
else
    OS := $(shell sh -c 'uname 2>/dev/null || echo Unknown')
endif

.PHONY: poetry

poetry:
ifeq ($(OS),Windows)
	-powershell (Invoke-WebRequest -Uri https://install.python-poetry.org -UseBasicParsing).Content | py -
	poetry self update
else ifeq ($(OS),Darwin)
	brew install poetry
	poetry self update
else
	-curl -sSL https://install.python-poetry.org | python3 -
	poetry self update
endif

@kezhenxu94
Copy link
Member

I cannot run this and failed to find a workable solution in Google, I did found some similar questions/issues but none works for me. Haven't dig deep to see why and how to fix this yet.

Seems like a macOS's built-in python (not for other python versions) issue here python-poetry/install.python-poetry.org#24

There isn't a final fix yet, but installing poetry from homebrew looks like an easy workaround for now. (Can you see if it works?) If so we could do something like this until they fix it?

ifeq ($(OS),Windows_NT)
    OS := Windows
else
    OS := $(shell sh -c 'uname 2>/dev/null || echo Unknown')
endif

.PHONY: poetry

poetry:
ifeq ($(OS),Windows)
	-powershell (Invoke-WebRequest -Uri https://install.python-poetry.org -UseBasicParsing).Content | py -
	poetry self update
else ifeq ($(OS),Darwin)
	brew install poetry
	poetry self update
else
	-curl -sSL https://install.python-poetry.org | python3 -
	poetry self update
endif

I'm not sure whether that works for everyone, but at least it doesn't work for me, I don't have homebrew installed.

@Superskyyy
Copy link
Member Author

I cannot run this and failed to find a workable solution in Google, I did found some similar questions/issues but none works for me. Haven't dig deep to see why and how to fix this yet.

I'm not sure whether that works for everyone, but at least it doesn't work for me, I don't have homebrew installed.

My bad I thought homebrew come in macOS as preinstalled. Then we have to use this then, which is among the official ways of installing poetry.

python3 -m pip install --user pipx
python3 -m pipx ensurepath
pipx install poetry
pipx upgrade poetry

@kezhenxu94
Copy link
Member

I cannot run this and failed to find a workable solution in Google, I did found some similar questions/issues but none works for me. Haven't dig deep to see why and how to fix this yet.

I'm not sure whether that works for everyone, but at least it doesn't work for me, I don't have homebrew installed.

My bad I thought homebrew come in macOS as preinstalled. Then we have to use this then, which is among the official ways of installing poetry.


python3 -m pip install --user pipx

python3 -m pipx ensurepath

pipx install poetry

pipx upgrade poetry

This looks good.

@@ -1,9 +1,13 @@
<!-- Uncomment the following checklist WHEN AND ONLY WHEN you're adding a new plugin -->
<!-- Fill the following checklists when a feature/plugin is added or modified-->
Copy link
Member

Choose a reason for hiding this comment

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

Contributors may just fill the checklist and keep the <!-- so these are not showing in the issue comment...

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you, now I suppose using the main repo's PR template will be a better choice, I added an additional plugin section there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changes pushed.

README.md Outdated Show resolved Hide resolved
tests/plugin/Dockerfile.plugin Outdated Show resolved Hide resolved
Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

Thanks!

@wu-sheng wu-sheng merged commit e87c7c6 into apache:master Nov 10, 2022
@Superskyyy Superskyyy deleted the migrate-poetry branch November 14, 2022 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Project chores documentation Improvements or additions to documentation test test
Projects
None yet
4 participants