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

docs: hoist ADR status #1407

Merged
merged 6 commits into from
Feb 22, 2023
Merged

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Feb 20, 2023

Addresses #1161 (comment)

Questions

  1. Do we want to make the same change across other celestiaorg repos?
  2. Should we consider an org wide ADR template?
    1. celestiaorg ADR template #1415

@rootulp rootulp added the documentation Improvements or additions to documentation label Feb 20, 2023
@rootulp rootulp self-assigned this Feb 20, 2023
@MSevey MSevey requested a review from a team February 20, 2023 18:59
@rootulp rootulp requested review from musalbas and removed request for adlerjohn, liamsi and MSevey February 20, 2023 19:00

Accepted -> Does not affect the Celestia Core Specification

Optimization 1 & 2 **Declined** as it is currently not worth it to introduce extra complexity for reducing the PFB proof size by 512-1024 bytes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bit messy imo to have half-accepted ADRs..

@musalbas
Copy link
Member

musalbas commented Feb 21, 2023

Left some general comments about inconsistent usage of status message but it doesn't have to block this PR which just moves the status field up.

Do we want to make the same change across other celestiaorg repos?

Yes but it's not urgent.

Should we consider an org wide ADR template?

I think all things called ADR in the org should follow the same template. If they follow a radically different template, they should be called something other than an ADR, to avoid confusion. cc @MSevey @liamsi

@MSevey MSevey requested a review from a team February 21, 2023 14:38
@rootulp
Copy link
Collaborator Author

rootulp commented Feb 21, 2023

I looked through Github repos in celestiaorg but couldn't find an adequate place for an ADR template. @MSevey or @liamsi do you have a suggestion for where such an ADR template should live?

@MSevey
Copy link
Member

MSevey commented Feb 21, 2023

I looked through Github repos in celestiaorg but couldn't find an adequate place for an ADR template. @MSevey or @liamsi do you have a suggestion for where such an ADR template should live?

Since the ADRs are files, I don't know if any GitHub features for the templates, like issue templates for example.

The simplest thing would be to just have an adr_template.md file in each repo. It's mostly a one time cost to make the template in all the repos that need it.

@rootulp
Copy link
Collaborator Author

rootulp commented Feb 21, 2023

We already have an ADR template per repo so changes to the ADR template (like the one in this PR) would need to be re-applied across all repos. I was proposing a celestiaorg wide repo that contains one ADR template. It may also be useful for things like linter rules where we establish an org wide standard.

@rootulp rootulp requested review from rach-id and musalbas February 21, 2023 15:16
@nashqueue
Copy link
Member

We should maybe include authors / Co-authors in ADRs so they can be pinged in future references

@evan-forbes
Copy link
Member

org wide template would be cool

@rootulp rootulp requested a review from rach-id February 22, 2023 14:19
Copy link
Member

@rach-id rach-id left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, would defer to other reviewers for final approval

@rootulp rootulp merged commit 8bd0f99 into celestiaorg:main Feb 22, 2023
@rootulp rootulp deleted the rp/hoist-adr-status branch February 22, 2023 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants