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

models: Do not make references for private symbols #718

Merged
merged 1 commit into from
Jun 18, 2021

Conversation

vinzenz
Copy link
Member

@vinzenz vinzenz commented Jun 16, 2021

Previously there was a reference for Models created for every symbol
attempted to being imported from leapp.models, however pytest tries to
import _pytestfixturefunction which is not a model obviously.
To work around pytest causing failures because of that, private symbols
are being filtered from the model reference creation magic.

For reference, this is what you would see running pytest without this fix:

....
INTERNALERROR>     self._inner_hookexec = lambda hook, methods, kwargs: hook.multicall(
INTERNALERROR>   File "/usr/local/lib/python3.9/site-packages/pluggy/callers.py", line 208, in _multicall
INTERNALERROR>     return outcome.get_result()
INTERNALERROR>   File "/usr/local/lib/python3.9/site-packages/pluggy/callers.py", line 80, in get_result
INTERNALERROR>     raise ex[1].with_traceback(ex[2])
INTERNALERROR>   File "/usr/local/lib/python3.9/site-packages/pluggy/callers.py", line 187, in _multicall
INTERNALERROR>     res = hook_impl.function(*args)
INTERNALERROR>   File "/home/vfeenstr/leapp-repository/conftest.py", line 45, in pytest_collectstart
INTERNALERROR>     _load_and_add_repo(collector.session.leapp_repository, current_repo_basedir)
INTERNALERROR>   File "/home/vfeenstr/leapp-repository/conftest.py", line 31, in _load_and_add_repo
INTERNALERROR>     manager.repo_by_id(repo_id).load(skip_actors_discovery=True)
INTERNALERROR>   File "/home/vfeenstr/.local/lib/python3.9/site-packages/leapp/repository/__init__.py", line 135, in load
INTERNALERROR>     resolve_model_references()
INTERNALERROR>   File "/home/vfeenstr/.local/lib/python3.9/site-packages/leapp/models/__init__.py", line 201, in resolve_model_references
INTERNALERROR>     reference.resolve()
INTERNALERROR>   File "/home/vfeenstr/.local/lib/python3.9/site-packages/leapp/models/__init__.py", line 188, in resolve
INTERNALERROR>     raise ModelDefinitionError('Undefined Model "{}"'.format(cls._referenced))
INTERNALERROR> leapp.exceptions.ModelDefinitionError: Undefined Model "_pytestfixturefunction"

@pirat89
Copy link
Member

pirat89 commented Jun 17, 2021

@vinzenz I know we are in time pressure, so in case you want to skip unit-tests now in this PR, please add at least TODO comment and create jira ticket. Regarding we are going to use multiple leapp repositories basically all the time, it will be better to have this field covered by tests - at least or bugs covered by tests.

@vinzenz vinzenz force-pushed the no-private-model-references branch 3 times, most recently from 42a443b to 76c9829 Compare June 17, 2021 08:31
Previously there was a reference for Models created for every symbol
attempted to being imported from leapp.models, however pytest tries to
import `_pytestfixturefunction` which is not a model obviously.
To work around pytest causing failures because of that, private symbols
are being filtered from the model reference creation magic.

Signed-off-by: Vinzenz Feenstra <vfeenstr@redhat.com>
@vinzenz vinzenz force-pushed the no-private-model-references branch from 76c9829 to 84e794b Compare June 17, 2021 11:26
@vinzenz
Copy link
Member Author

vinzenz commented Jun 17, 2021

@vinzenz I know we are in time pressure, so in case you want to skip unit-tests now in this PR, please add at least TODO comment and create jira ticket. Regarding we are going to use multiple leapp repositories basically all the time, it will be better to have this field covered by tests - at least or bugs covered by tests.

While I can add a test, this is supposed to fix problems in leapp repository and these problems are somehow caused by pytest. I don't see this a big problem as the coverity of what is supported is covered and doesn't break.

I also made travis work again so finally it builds on travis - As they moved now to travis-ci.com instead of travis-ci.org for opensource code...

In another PR I am working on moving to github actions for unit tests anyway.

Copy link
Member

@Rezney Rezney left a comment

Choose a reason for hiding this comment

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

The fix is working fine and does not seem to affect anything else.

@Rezney Rezney merged commit 5f7ebdc into oamg:master Jun 18, 2021
pirat89 added a commit to pirat89/leapp that referenced this pull request Oct 19, 2021
## Packaging
- Drop the dependency on leapp-repository for Fedora and RHEL 8+ (oamg#717)
- Provide builds for RHEL 7+ and Fedora (oamg#717)
- Drop automatically generated Python dependences on RHEL 8+ and Fedora systems (oamg#717, oamg#716)
- Bump the provided leapp-framework capability to 2.0 (oamg#700)

## Framework
### Fixes
- models: Do not make references to private symbols (oamg#718)

### Enhancements
- Introduce the LEAPP_DEVEL_DATABASE_SYNC_OFF envar to enable speed up writes into the leapp database by disabling synchronisation - only for development / testing purposes (oamg#732)

## Leapp
### Fixes
- Fix print of the leapp help msg for Python 3.6+ (oamg#731)

### Enhancements
- The leapp commands are now defined/provided by leapp-repositories; leapp discovers the specified commands automatically (oamg#700)
- Add CLI support for `choices` and `default` for options of leapp commands (oamg#734)

## Modifications
- Makefile: Added the `fast_lint` target to apply linters just on files different from master (oamg#733)
@pirat89 pirat89 mentioned this pull request Oct 19, 2021
pirat89 added a commit to pirat89/leapp that referenced this pull request Oct 19, 2021
## Packaging
- Drop the dependency on leapp-repository for Fedora and RHEL 8+ (oamg#717)
- Provide builds for RHEL 7+ and Fedora (oamg#717)
- Drop automatically generated Python dependences on RHEL 8+ and Fedora systems (oamg#717, oamg#716)
- Bump the provided leapp-framework capability to 2.0 (oamg#700)

## Framework
### Fixes
- models: Do not make references to private symbols (oamg#718)

### Enhancements
- Introduce the LEAPP_DEVEL_DATABASE_SYNC_OFF envar to enable speed up writes into the leapp database by disabling synchronisation - only for development / testing purposes (oamg#732)

## Leapp
### Fixes
- Fix print of the leapp help msg for Python 3.6+ (oamg#731)

### Enhancements
- The leapp commands are now defined/provided by leapp-repositories; leapp discovers the specified commands automatically (oamg#700)
- Add CLI support for `choices` and `default` for options of leapp commands (oamg#734)

## Modifications
- Makefile: Added the `fast_lint` target to apply linters just on files different from master (oamg#733)
pirat89 added a commit that referenced this pull request Oct 19, 2021
## Packaging
- Drop the dependency on leapp-repository for Fedora and RHEL 8+ (#717)
- Provide builds for RHEL 7+ and Fedora (#717)
- Drop automatically generated Python dependences on RHEL 8+ and Fedora systems (#717, #716)
- Bump the provided leapp-framework capability to 2.0 (#700)

## Framework
### Fixes
- models: Do not make references to private symbols (#718)

### Enhancements
- Introduce the LEAPP_DEVEL_DATABASE_SYNC_OFF envar to enable speed up writes into the leapp database by disabling synchronisation - only for development / testing purposes (#732)

## Leapp
### Fixes
- Fix print of the leapp help msg for Python 3.6+ (#731)

### Enhancements
- The leapp commands are now defined/provided by leapp-repositories; leapp discovers the specified commands automatically (#700)
- Add CLI support for `choices` and `default` for options of leapp commands (#734)

## Modifications
- Makefile: Added the `fast_lint` target to apply linters just on files different from master (#733)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants