docs: update oep-49 for clarity around new apps #455

rgraber · 2023-03-01T14:05:09Z

For anyone looking at this in the future:
Even though this PR says "Closed", it was in fact Merged.

Update OEP-49 by

moving it to Best Practices
adding a section for the ADR that should be included
adding a change history

Note this will be kept as 2 separate commits for merge for cleaner history.

robrap

Thank you! Also, I didn't see you post this review in the community channels. Were you planning to do that after you have internal approval?

robrap · 2023-03-01T14:58:41Z

oeps/best-practices/oep-0049-django-app-patterns.rst

@@ -267,7 +267,31 @@ Do note that even if you expose your tasks through ``api.py`` to be used by othe
 still be configured with the ``tasks`` import name, as the celery identifier for your task (as set by the celery
 decorator) is based off the original file.

+docs/decisions/0001-purpose-of-this-repo.rst or 0001-purpose-of-this-app.rst


Why repo? This is all about the app, right?

Suggested change

docs/decisions/0001-purpose-of-this-repo.rst or 0001-purpose-of-this-app.rst

docs/decisions/0001-purpose-of-this-app.rst

Actually that's an interesting point. If a new repo has only one new app, do we want both a purpose of this app and a purpose of this repo?

My gut reaction is that requring separate ADRs for the new repo and the initial apps sounds like too much paperwork. However, requiring ADRs for apps created in exisitings repo does sound fair to me.

As a conpromise, we could recommend that the initial app(s) have stub ADRs, each of which point to the repo-level ADR. The repo cookiecutter could be updated to provide that.

Yeah, I'd lost track of this being for apps in general, which might be in their own repos. If it's a single-app repo, I think there should just be one docs/ dir, either at the repo root or inside the app. Top-level sounds more convenient and easy to notice. The one reason I can think of to put it inside the app is in case the code moves into another repo (maybe absorbed into an IDA?), since having it at the repo root might risk it being lost. But I don't know how plausible that is.

I think @kdmccormick 's suggestion of the stub ADR pointing to the main ADR addresses most of your concerns @timmc-edx . The documentation is effectively centralized, but if someone moves the app folder they should notice that something is up when their link breaks.

A stub makes to me as well, yeah.

For what it's worth, I wrote that proposal thinking about apps in an IDA, not single-app packages.

I would agree with @timmc-edx's earlier point that a single ADR in the root dir of the a single-app package is probably good enough. But if that's not that the consensus that's been reached here, that's fine with me too.

Updated with the stub-ADR-in-the-app version. Even I find the phrasing I used confusing so if anyone has ideas for ways to express this more clearly I'm very much open to them.
I will squash the commits before merging.

[inform] The updated text is in https://github.com/openedx/open-edx-proposals/pull/455/files#r1122281928, but the PR hasn't been updated yet.

PR has been updated

oeps/best-practices/oep-0049-django-app-patterns.rst

timmc-edx · 2023-03-01T15:15:28Z

Agreed with Robert's recommendations -- beyond those, looks good to go.

robrap · 2023-03-01T20:46:28Z

oeps/best-practices/oep-0049-django-app-patterns.rst

+
+This should be an architectural decision record (ADR) describing the decision behind adding the app. Future ADRs should also be placed in this directory. See more about what should go in ADRs in the `ADRs section of OEP-0019`_
+
+If this is a repository that only contains a single app, the information should be in a file called `0001-purpose-of-this-repo.rst`. There should be a `0001-purpose-of-this-app.rst` ADR in application subdirectory (in its own docs/decisions subdirectory) that just points to the repository-level one.


I stick with my original proposal to update the title of this section to docs/decisions/0001-purpose-of-this-app.rst. All of the other sections are relative to the app directory.

I'm wondering if this could be simpler, and not necessarily require (0001-purpose-of-this-repo.rst), since this OEP isn't about that. How do you feel about something like the following?

If this is the only app in the entire repo, this ADR could refer to a ``0001-purpose-of-this-repo.rst`` in the repository-level docs/decisions directory.

I would replace "could" with "should" but that's fine by me

@rgraber: optional: You could reference https://github.com/openedx/edx-cookiecutters#3-todos-after-running-cookiecutter if you'd like.

I approve once this has been updated.

I recommend "[could/should] be a stub that links to" over "should refer to". I think the latter leaves room for misintepretation (ie "do I have to write a whole new ADR, that happens to contain a link to the repo ADR?")

kdmccormick · 2023-03-02T18:34:38Z

oeps/best-practices/oep-0049-django-app-patterns.rst

+
+This should be an architectural decision record (ADR) describing the decision behind adding the app. Future ADRs should also be placed in this directory. See more about what should go in ADRs in the `ADRs section of OEP-0019`_
+
+If this is the only app in the repository, this ADR should just be a stub linking to the full ADR in `0001-purpose-of-this-repo.rst` (see `TODOs after running cookiecutter`_).


Rather than adding this to the post-cookiecutter TODO list, could we add it to the cookiecutters as a part of this OEP change?

As in postpone this PR until we have updated the cookiecutter?

Yeah, or at least until we have a draft PR ready to go for updating the cookiecutter. Since we are asking folks to make these additional ADRs starting now, my intuition is that it'd be best to provide the cookiecutter scaffolding now as well.

I don't know, maybe I'm underestimating the effort it'd take to update the cookiecutter. Don't take this as a blocking suggestion; what you have here is OK with me.

I'm nervous about blocking this, so I'll leave this as is and then make an issue to add this to the cookiecutter if there isn't one already. If there is one I'll add a task to update this line.

I don't see the benefit of blocking on this. The OEP states what someone should do. Cookiecutter changes may help automate some of that, but I bet there are a ton of automation improvements in our OEPs, and this is just one.

Also, one could argue that adding the stub automatically will make it less likely that anyone will update it in the many cases where it should be updated. An alternative would be to add a codeblock here with an example link, if someone works out the rST reference that would work. Note, this too could come later.

This is now openedx/edx-cookiecutters#299. We can further discuss there.

kdmccormick

I have a suggestion in this thread, but it's not blocking. Thanks for the update!

robrap · 2023-03-02T19:59:54Z

oeps/best-practices/oep-0049-django-app-patterns.rst

+docs/decisions/0001-purpose-of-this-app.rst
+===========================================
+
+This should be an architectural decision record (ADR) describing the decision behind adding the app. Future ADRs should also be placed in this directory. See more about what should go in ADRs in the `ADRs section of OEP-0019`_


Nit: Missing period at the end.

docs: update oep-49 for clarity around new apps

docs: move oep-49 to best practices and add history

0b1f15b

rgraber force-pushed the rsgraber/20230301-move-oep-49 branch 2 times, most recently from 7a1a48f to 0e1bd18 Compare March 1, 2023 14:33

rgraber marked this pull request as ready for review March 1, 2023 14:34

rgraber force-pushed the rsgraber/20230301-move-oep-49 branch from b220e53 to 6dc6839 Compare March 1, 2023 14:36

robrap reviewed Mar 1, 2023

View reviewed changes

rgraber force-pushed the rsgraber/20230301-move-oep-49 branch 3 times, most recently from 6a825b4 to 27a510b Compare March 1, 2023 17:24

robrap reviewed Mar 1, 2023

View reviewed changes

timmc-edx mentioned this pull request Mar 1, 2023

Enforcement for ADR 14 openedx/edx-platform#31865

Open

3 tasks

rgraber requested review from robrap and kdmccormick March 2, 2023 15:04

rgraber mentioned this pull request Mar 2, 2023

Improve clarity of No New Apps ADR openedx/edx-platform#31659

Closed

10 tasks

kdmccormick reviewed Mar 2, 2023

View reviewed changes

kdmccormick approved these changes Mar 2, 2023

View reviewed changes

robrap approved these changes Mar 2, 2023

View reviewed changes

docs: add recommendation for ADR

15d51a7

rgraber force-pushed the rsgraber/20230301-move-oep-49 branch from 4b44466 to 15d51a7 Compare March 2, 2023 20:03

Merge pull request #455 from openedx/rsgraber/20230301-move-oep-49

82b3eae

docs: update oep-49 for clarity around new apps

rgraber closed this Mar 3, 2023

rgraber deleted the rsgraber/20230301-move-oep-49 branch March 3, 2023 13:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: update oep-49 for clarity around new apps #455

docs: update oep-49 for clarity around new apps #455

rgraber commented Mar 1, 2023 •

edited by kdmccormick

Loading

robrap left a comment •

edited

Loading

robrap Mar 1, 2023

rgraber Mar 1, 2023

kdmccormick Mar 1, 2023 •

edited

Loading

timmc-edx Mar 1, 2023

rgraber Mar 1, 2023 •

edited

Loading

timmc-edx Mar 1, 2023

kdmccormick Mar 1, 2023

rgraber Mar 1, 2023 •

edited

Loading

robrap Mar 1, 2023

rgraber Mar 2, 2023

timmc-edx commented Mar 1, 2023

robrap Mar 1, 2023

rgraber Mar 1, 2023 •

edited

Loading

robrap Mar 1, 2023

robrap Mar 1, 2023

kdmccormick Mar 1, 2023

kdmccormick Mar 2, 2023

rgraber Mar 2, 2023 •

edited

Loading

kdmccormick Mar 2, 2023

rgraber Mar 2, 2023

robrap Mar 2, 2023

rgraber Mar 2, 2023

kdmccormick left a comment

robrap Mar 2, 2023

	docs/decisions/0001-purpose-of-this-repo.rst or 0001-purpose-of-this-app.rst
	docs/decisions/0001-purpose-of-this-app.rst


		This should be an architectural decision record (ADR) describing the decision behind adding the app. Future ADRs should also be placed in this directory. See more about what should go in ADRs in the `ADRs section of OEP-0019`_

		If this is a repository that only contains a single app, the information should be in a file called `0001-purpose-of-this-repo.rst`. There should be a `0001-purpose-of-this-app.rst` ADR in application subdirectory (in its own docs/decisions subdirectory) that just points to the repository-level one.


		This should be an architectural decision record (ADR) describing the decision behind adding the app. Future ADRs should also be placed in this directory. See more about what should go in ADRs in the `ADRs section of OEP-0019`_

		If this is the only app in the repository, this ADR should just be a stub linking to the full ADR in `0001-purpose-of-this-repo.rst` (see `TODOs after running cookiecutter`_).

docs: update oep-49 for clarity around new apps #455

docs: update oep-49 for clarity around new apps #455

Conversation

rgraber commented Mar 1, 2023 • edited by kdmccormick Loading

robrap left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdmccormick Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgraber Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgraber Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timmc-edx commented Mar 1, 2023

Choose a reason for hiding this comment

rgraber Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgraber Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdmccormick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgraber commented Mar 1, 2023 •

edited by kdmccormick

Loading

robrap left a comment •

edited

Loading

kdmccormick Mar 1, 2023 •

edited

Loading

rgraber Mar 1, 2023 •

edited

Loading

rgraber Mar 1, 2023 •

edited

Loading

rgraber Mar 1, 2023 •

edited

Loading

rgraber Mar 2, 2023 •

edited

Loading