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 dependency cycle between galaxy.security and galaxy.model. #7554

Merged
merged 3 commits into from Mar 19, 2019

Conversation

jmchilton
Copy link
Member

Without this PR:

>>> from galaxy.security import get_permitted_actions
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "galaxy/security/__init__.py", line 12, in <module>
    import galaxy.model
  File "galaxy/model/__init__.py", line 51, in <module>
    from galaxy.security import get_permitted_actions
ImportError: cannot import name get_permitted_actions

Previously galaxy.model import galaxy.security and galaxy.security import galaxy.model. This prevents certain seemingly reasonable imports from working properly (see above). The new break down of code is objectively less circular and matches the better pattern we've taken more recently of separating interfaces from implementations to allow cleaner imports and dependencies.

The fix is to use galaxy.security to define the interface for security operations on Galaxy components and moving the actual model dependent code into galaxy.model.security. Now galaxy.security has no dependencies on galaxy.model, galaxy.model (the raw model definitions) only depends on the interface and utilities methods, and the actual implementation of the security agent is found in galaxy.model.security that is effectively injected at runtime from galaxy.model.mapping.

@jmchilton jmchilton added kind/bug kind/refactoring cleanup or refactoring of existing code, no functional changes area/cleanup labels Mar 18, 2019
@dannon
Copy link
Member

dannon commented Mar 18, 2019

Makes sense to me; can you do this refactoring with git mv, though, so we keep the history intact?

@jmchilton
Copy link
Member Author

I think Github is being confusing here - if you load the diff the original file is still there - I didn't move or copy the file, I just pulled out part of it. That said I guess I pulled out a majority of it so it might make sense to conceptually think about moving it and then extracting the interface instead of extracting the implementation. I'll see if that works...

@dannon
Copy link
Member

dannon commented Mar 18, 2019

@jmchilton Yep, no confusion, that's what I meant. Since the majority of that code relocated, I'd do the mv first and extract what you needed in the module to preserve the most history we could.

@jmchilton
Copy link
Member Author

If you move a file and then add another file in the old file's place all in one commit it seems it ignores the mv. I'll break it into a broken commit and a commit to fix it and see if that recovers the history.

```python
>>> from galaxy.security import get_permitted_actions
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "galaxy/security/__init__.py", line 12, in <module>
    import galaxy.model
  File "galaxy/model/__init__.py", line 51, in <module>
    from galaxy.security import get_permitted_actions
ImportError: cannot import name get_permitted_actions
```

Previously galaxy.model import galaxy.security and galaxy.security import galaxy.model. This prevents certain seemingly reasonable imports from working properly (see above). The new break down of code is objectively less circular and matches the better pattern we've taken more recently of seperating interfaces from implementations to allow cleaner imports and dependencies.

The fix is to use galaxy.security to define the interface for security operations on Galaxy components and moving the actual model dependent code into galaxy.model.security. Now galaxy.security has no dependencies on galaxy.model, galaxy.model (the raw model definitions) only depends on the interface and utilities methods, and the actual implementation of the security agent is found in galaxy.model.security that is effectively injected at runtime from galaxy.model.mapping.
Breaking one commit into two to try to preserve history.
@jmchilton
Copy link
Member Author

Blame is still broken on github for my viewing of this PR but it works locally for me on this branch.

@dannon
Copy link
Member

dannon commented Mar 18, 2019

@jmchilton Thanks for doing that -- it looks good to me, too. Might not always be a big deal, but I figured especially for the security module it should be as easy as possible to track changes, so I appreciate it.

@jmchilton
Copy link
Member Author

Totally makes sense @dannon, thanks for the catch!

@galaxybot galaxybot added this to the 19.05 milestone Mar 18, 2019
@hexylena
Copy link
Member

Oh, hey, we encountered this on eu during admin training. @natefoo helped patch it. No idea why it only affected us at the time, sorry, we hadn't upstreamed our patch yet!

@dannon dannon merged commit ac3c650 into galaxyproject:dev Mar 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug kind/refactoring cleanup or refactoring of existing code, no functional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants