-
Notifications
You must be signed in to change notification settings - Fork 33
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
docs: Add an ADR for using Backstage to support OEP-55 #343
Conversation
oeps/processes/oep-0055/decisions/0001-use-backstage-to-support-maintainers.rst
Show resolved
Hide resolved
oeps/processes/oep-0055/decisions/0001-use-backstage-to-support-maintainers.rst
Outdated
Show resolved
Hide resolved
oeps/processes/oep-0055/decisions/0001-use-backstage-to-support-maintainers.rst
Outdated
Show resolved
Hide resolved
oeps/processes/oep-0055/decisions/0001-use-backstage-to-support-maintainers.rst
Show resolved
Hide resolved
9df2dfe
to
156868b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple comments on the specifics, but the overall proposal sounds great to me and I'm excited to try out Backstage 🎉
name: 'docs.openedx.org' | ||
description: "This repository contains the root documentation site for https://docs.openedx.org" | ||
links: | ||
- url: "https://github.com/openedx/docs.openedx.org" | ||
title: "Source Code" | ||
icon: "Code" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name, description, and source code URL as strike me as redundant with the repository's name, GitHub description, and GitHub URL. Is there anything we can do to de-dupe this or lint to keep it in sync?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it could be baked into logic for the templates we start from when making new files?
See also, placeholders in yaml: https://stackoverflow.com/questions/41620674/use-placeholders-in-yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a bad example I think from when I was experimenting with the contents of the files. I'll update it to a link to the deployed site which might be a better example.
oeps/processes/oep-0055/decisions/0001-use-backstage-to-support-maintainers.rst
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking forward to the new tools.
discoverability and maintainership burdens, the Open edX community shall | ||
maintain a `backstage`_ instance and related metadata. | ||
|
||
Consequences |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 2U/edX were maintaining much of this information in a spreadsheet before the tCRIL-split. This ADR could acknowledge the need to update this existing tooling. Even if not mentioned in the ADR, this work needs to be communicated and planned for.
- The existing spreadsheet also has exceptional maintenance requirements for the edx-platform monolith. Does tCRIL plan on supporting that level of detail, or is it planning on having edx-platform maintained by 2U/edX, with the split ownership being decided/documented/maintained by 2U/edX as we see fit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2U/edX were maintaining much of this information in a spreadsheet before the tCRIL-split. This ADR could acknowledge the need to update this existing tooling. Even if not mentioned in the ADR, this work needs to be communicated and planned for.
That feels like a consequence internal to 2U and not one for the open source project. I agree the work needs to be done either way, but I'm not sure mentioning it here is valuable from the open source project perspective.
The existing spreadsheet also has exceptional maintenance requirements for the edx-platform monolith. Does tCRIL plan on supporting that level of detail, or is it planning on having edx-platform maintained by 2U/edX, with the split ownership being decided/documented/maintained by 2U/edX as we see fit?
I think edx-platform maintainership is being avoided for the pilot but we will eventually need to make a plan for it. Given its complexity, I expect there will be a future ADR that addresses its ownership specifically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robrap maybe we can touch base over slack about the nature of the maintenance requirements? I'm curious how much of it would be relevant to any organization maintaining edx-platform
vs 2U specifically.
oeps/processes/oep-0055/decisions/0001-use-backstage-to-support-maintainers.rst
Outdated
Show resolved
Hide resolved
title: "Source Code" | ||
icon: "Code" | ||
annotations: | ||
openedx.org/arch-interest-groups: "feanil" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do we find the explanation of what "annotations" means? Same for "spec", etc. Is this defined by Backstage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can find it here, I linked to it from just above this code-block in the references section. Let me know if you think I need more specific stuff within the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I see the backstage docs now, but I have a feeling that the pure generality of their docs could be narrowed with some specifics for our use of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add another comment here, let's see if that will help. Happy to add a lot more text here to help people understand what's up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nedbat does this address your concerns and provide sufficient context or would you like more words?
owner: group:docs.edx.org-maintainers | ||
|
||
# (Required) Acceptable Type Values: service, website, library | ||
type: 'website' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does "type" relate to "kind"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More details on the system model are here: https://backstage.io/docs/features/software-catalog/system-model
I'll add it as a reference link.
oeps/processes/oep-0055/decisions/0001-use-backstage-to-support-maintainers.rst
Outdated
Show resolved
Hide resolved
oeps/processes/oep-0055/decisions/0001-use-backstage-to-support-maintainers.rst
Show resolved
Hide resolved
* As a result of adding the ``catalog-info.yaml`` file we end with two top-level | ||
files that have very similar data. If this decision is not reversed after the | ||
initial pilot, it may make sense to revisit this consequence and see if we can | ||
consolidate this metadata into a single file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd also need to update process to ensure new CCs are added to the relevant catalog-info.yaml
files. Or better yet, make that part of the CC's onboarding via an OSPR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pomegranited would this not be covered by the process to elect and assign maintainers generally?
|
||
Rejected Alternatives | ||
********************* | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were there no alternatives? If there are no comparable OOTB options we could perhaps note briefly why we don't want to build it ourselves out of scripts and spreadsheets, for instance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hurtstotouchfire do you mean were there no alternatives services or out of box tools? If so, there are a few commercial products but their use case tends to be different. They are oriented towards protecting the content rather than making it visible to everyone so often their pricing models and workflows don't make sense for an opensource project. This felt a bit outside of the requirements of what we were looking for so I did not initially mention it since they were not viable alternatives for the basic requirement of everyone needs to be able to see it.
|
||
* Harder to aggregate the data from the unstructured files in a way that would | ||
allow us to easily build a `single pane of glass`_ where maintainers can see | ||
all the things they own. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason I have in my memory something about using github repo tags for this? I think it was @ormsbee's idea. In any case, would we want to integrate that with Backstage somehow or are they just separate concerns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully understand what you're saying here but feel free to suggest text that you'd like here or another rejected alternative that you would want to document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your memory serves you right @hurtstotouchfire -- on the OEP-57 draft, Dave mentioned using GH topics to group repositories into tiers like Core, Supporting, etc. Repo topics are an interesting way of storing metadata in a GH-native way that you might consider employing as you iterate on this @feanil .
Correct me if I'm wrong, but I get the sense that you are hoping to get this ADR over the line so that we can experiment with actually using backstage for the maintainer pilot. After we use backstage for a while it'll probably be easier to tell which GH features we do and don't want to tie in with, as well as what exactly the schema should look like. That is to say: I'm fine if you're not explicitly rejecting any alternatives at the moment, since we're not exactly sure what this thing will look like and how everyone will use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, although I share @nedbat 's question on Type vs Kind
Consider merging as "Provisional" instead of "Accepted" as it seems like you're hoping to try to this out on a handful of repos and iterate on the spec.
81faf0f
to
3190698
Compare
Since this is the first ADR under oep-55 I also added the relevant directory structure and wired it up into the OEP rst.
3190698
to
54fddc6
Compare
Thanks for all the feedback everyone. I'll be merging this in as |
Since this is the first ADR under oep-55 I also added the relevant
directory structure and wired it up into the OEP rst.