Declare arches applications in AppConfig #11009

jacobtylerwalls commented 1 month ago

Types of changes

[ ] Bugfix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[x] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of Change

If we shift the burden from declaring arches applications from the installer to the publisher, then the installer can just put it in INSTALLED_APPS like any django app. We can then presumably extend on this pattern to let the publisher of the app declare whether their app has graphs, business_data, etc., without any further friction for the installer.

The publisher would do this in apps.py:

from django.apps import AppConfig

class MyArchesApp(AppConfig):
    name = 'my_arches_app'
    verbose_name = 'My Arches App'
    is_arches_application = True

Issues Solved

Closes #11009

Checklist

I targeted one of these branches:
- [x] dev/7.6.x (under development): features, bugfixes not covered below
- [ ] dev/7.5.x (main support): regressions, crashing bugs, security issues, major bugs in new features
- [ ] dev/6.2.x (extended support): major security issues, data loss issues
[x] I added a changelog in arches/releases
[ ] I submitted a PR to arches-docs (if appropriate)
[ ] Unit tests pass locally with my changes
[ ] I added tests that prove my fix is effective or that my feature works
[ ] My test fails on the target branch

Ticket Background

Sponsored by: Getty Conservation Institute
Found by: @jacobtylerwalls
Tested by: @
Designed by: @

Further comments

chrabyrd commented 1 month ago

@jacobtylerwalls

I appreciate all the hard work you've put into this PR. As it’s taking shape, I’m starting to feel that we might be adding more complexity than necessary, and it may be overall better to just stick with the settings declaration. I’d love to hear your thoughts on this and I'm for sure open to discussion.

jacobtylerwalls commented 1 month ago

I’m starting to feel that we might be adding more complexity than necessary, and it may be overall better to just stick with the settings declaration. I’d love to hear your thoughts on this and I'm for sure open to discussion.

I hear that. Let me take one last go at removing the requirement at a specific INSTALLED_APPS ordering and see how it looks after that.

jacobtylerwalls commented 2 weeks ago

The expectation is that I would then see bar's console log. However I see arches-core console log.

Thanks for the repro steps -- I followed it and got the same result.

Then I retested on dev/7.6.x, and I found that in the sanity-check step before deleting any files, I see my foo log statement, when we would expect bar. And then removing and rebuilding obviously falls back to core.

So, my impression is that we're improving the situation slightly with this PR. Do you see the same?

jacobtylerwalls commented 2 weeks ago

(Hm, if project files are supposed to overwrite arches applications files, perhaps the new behavior isn't really an improvement.)

chrabyrd commented 1 week ago

Ah -- yeah sanity checks for the win. Once https://github.com/archesproject/arches/pull/11147 is merged into dev/7.5.x and dev/7.5.x is merged into dev/7.6.x and then those changes are merged/tested here, we should be g2g 👍

jacobtylerwalls commented 1 week ago

Retested the scenario from above, looking clear now 😎

jacobtylerwalls commented 1 week ago

Ah I thought we had consensus on collapsing the app/project distinction, but no problem to cleave that off here and propose it separately to vet it throughly 👍

I'll just flip the bit in the apps.py template to set is_arches_application = False and just document that when you're ready to graduate to a reusable app you need to flip the bit.

Hopefully flipping the bit will become unnecessary!

chrabyrd commented 1 week ago

Ah I thought we had consensus on collapsing the app/project distinction, but no problem to cleave that off here and propose it separately to vet it throughly 👍

I'll just flip the bit in the apps.py template to set is_arches_application = False and just document that when you're ready to graduate to a reusable app you need to flip the bit.

Hopefully flipping the bit will become unnecessary!

Ah I may have missed that. Ideally I'd rather have the settings_util updated to filter our the project its called from, but if that's not kosher I'd much rather have it as it currently is instead of needing the user to flip a bit to make a project an Arches app

jacobtylerwalls commented 1 week ago

Oh perfect, I'll do that.

Then I would want to clarify the settings.py <--> webpack interface to use a key like "arches applications other than this one/app root" (overly descriptive ? 😛 ) so we can keep using is_arches_application in apps.py to represent "anything built with arches", no bit flipping necessary.

Then we can decide what we mean by "arches application" -- do we mean "anything you build with arches"? (my hope)

chrabyrd commented 1 week ago

Oh fair! Yeah I'm down to update the key to something like EXTERNAL_ARCHES_APPLICATIONS.

But taking a step back, this all may be unnecessary. I'd say don't a change a thing today, I'd like some time to think if what we currently have will break anything. Because the more I think on it, the more sorta like having the project also be counted as an Arches application 👍

Apologies for the run-around 😅

jacobtylerwalls commented 1 week ago

No worries. My expectation is that it will give implementers a little more control over what overrides what via just reading from the order of INSTALLED_APPS. We can provide sensible defaults, but ultimately the implementer can do whatever they want. (Should feel natural to them if coming from a Django background, which was my takeaway from the convo after core standup the other day.)

jacobtylerwalls commented 5 days ago

Thanks for the review @chrabyrd!

archesproject / arches