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

Fix cascade dependency plugins #1519

Merged
merged 6 commits into from
Jul 11, 2021

Conversation

Nov1kov
Copy link
Contributor

@Nov1kov Nov1kov commented Jun 19, 2021

Bug reproduce

Complicated plugin structure.

Parent2 
   |
   V
Parent1
    /\
   /  \----> Child2
  V
Child1

It works correctly after load on my MacBook. But didn't after deploy & run in Docker on a Linux machine.
Plugin "Parent1" can't find Childs plugins, by method get_plugin.

Research

This behavior depends on a file system. Method pathlib.Path.glob works differently on Linux & Mac. Bot load plugins depending on the order of the files.
PluginManager can activate plugins by direct load, or if the plugin is DependOn. So if the plugin activates by the second way, the plugin doesn't set depends list.

Merge request

  1. Rreproduced bug by unit tests.
  2. Fixed _activate_plugin_dependencies in PluginManager.

@sijis
Copy link
Contributor

sijis commented Jun 20, 2021

Do you think this bug and fix is also related to #1505?

@Nov1kov
Copy link
Contributor Author

Nov1kov commented Jun 20, 2021

@sijis Hello, I'm not sure. In my case, I have exception: Plugin dependency Child1 needs to be listed in section [Core] key "DependsOn" to be used in get_plugin..
My unit test shows root of the problemm.
So, I can add unit test like from #1397, and we'll see about that.

@sijis
Copy link
Contributor

sijis commented Jun 21, 2021

So, I can add unit test like from #1397, and we'll see about that.

If you can, that would be extremely helpful.

@Nov1kov
Copy link
Contributor Author

Nov1kov commented Jun 21, 2021

@sijis

  1. I Fixed my unit tests for Linux file system.
  2. Added unit test from Circular Dependencies when there are none #1397 @jakubclark please verify your unit test. I can't reproduce the issue on my Mac. It works ok.
  3. Please rerun tests on CI.

@Nov1kov
Copy link
Contributor Author

Nov1kov commented Jun 24, 2021

@sijis code-style fixed!

@Nov1kov
Copy link
Contributor Author

Nov1kov commented Jul 8, 2021

Hello maintainers!
@gbin @sijis @zoni what about the merge request?
I must use my fork.

@sijis
Copy link
Contributor

sijis commented Jul 8, 2021

I should have from free cycles this weekend to look at this more in-depth.

@sijis sijis force-pushed the fix_cascade_dependency_plugins branch from 2088a2b to bcd7f1e Compare July 11, 2021 06:28
@sijis sijis merged commit c81c30d into errbotio:master Jul 11, 2021
@sijis
Copy link
Contributor

sijis commented Jul 11, 2021

Thank you again for the fixes. I truly appreciate the amount of testing included.

sijis added a commit that referenced this pull request Jun 11, 2022
* reproduce bug by unittest

* fix lose sub depends

* fix unit tests for linux systems

* add unit test from #1397

* fix codestyle

* docs: Add fixes to CHANGES

Co-authored-by: Ivan Novikov <i.novikov@cardsmobile.ru>
Co-authored-by: Sijis Aviles <sijis.aviles+github@gmail.com>
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.

None yet

2 participants