Declare arches applications in AppConfig #11009

[jacobtylerwalls]

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• 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:
  • 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
• 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]

I know this is still a draft but I saw you requested a review. Overall looks good, haven't given it a spin yet but I can grok what's going on.

[chrabyrd]

Overall getting there but there's still some stuff to work out IMO

[chrabyrd]

@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]

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.

[chrabyrd]

Ok this is looking pretty solid. There's a few things I've brought up and I'll kick the tires once more before approving if we choose to stay on this path 👍

[chrabyrd]

overall looking good just a few last things

[chrabyrd]

small nits in release doc, otherwise I'm happy with how this turned out from a design perspective.

One major bug I found is there seems to be an issue with static files.

Repro steps:

• Make a project ( foo )
• Make a second project ( bar )
• Install bar in your virtual ENV
• Add bar to INSTALLED_APPS inside foo
• In foo's media/reports/default.js add a console log
• In bar's media/reports/default.js add a different console log
• For kicks, in arches core media/reports/default.js add yet another console log
• in foo, build the static bundle, create a dummy graph/resource instance and navigate to the report. It should show the console log in foo's default.js
• delete foo/media/js/reports/default.js
• re-build the static bundle

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

[jacobtylerwalls]

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]

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

[chrabyrd]

Ah -- yeah sanity checks for the win. Once #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]

Retested the scenario from above, looking clear now 😎

[chrabyrd]

Hmm the regression goes away but now the project is coming across as an arches_application

Data imported from settings.py: {
  APP_ROOT: '/Users/cbyrd/Projects/foo/foo',
  ARCHES_APPLICATIONS: [ 'foo', 'bar' ],
  ARCHES_APPLICATIONS_PATHS: {
    foo: '/Users/cbyrd/Projects/foo/foo',
    bar: '/Users/cbyrd/Projects/bar/bar'
  },
  SITE_PACKAGES_DIRECTORY: '/Users/cbyrd/Projects/ENV/lib/python3.10/site-packages',
  PUBLIC_SERVER_ADDRESS: 'http://localhost:8000/',
  ROOT_DIR: '/Users/cbyrd/Projects/arches/arches',
  STATIC_URL: '/static/',
  WEBPACK_DEVELOPMENT_SERVER_PORT: 9000
}

which I almost don't hate. There could be conversation there where we really don't care about APP_ROOT and instead just treat any project like an Arches app. But IMO that could require some intense changes and is likely out-of-scope for this

[jacobtylerwalls]

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]

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]

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]

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]

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.)

[chrabyrd]

@jacobtylerwalls this looks good and I'm on board. Rolled a few changes into this having to do with the ordering of Project -> Arches Apps -> Arches core, and some polish around always having at least 1 Arches app now. Once the merge conflict has been resolved and you can confirm that this is still functioning how you'd like, this should be good to approve 👍

[chrabyrd]

Added this instruction, though I'm wondering if there's a better way to ensure ordering?

[jacobtylerwalls]

Nice. Let's see how this goes in practice: we can keep tinkering with the docs without it being a breaking change. I'm thinking it's the responsibility of the readme in the arches application to make this as clear as possible for their users.

[jacobtylerwalls]

Thanks for the review @chrabyrd!

[chrabyrd]

lgtm! 👍