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

Make use of the common adr template in the .github repo #1599

Merged
merged 4 commits into from
Apr 17, 2023
Merged

Conversation

MSevey
Copy link
Member

@MSevey MSevey commented Apr 5, 2023

Overview

To unify the ADR structure we added a template in the .github repo. This PR adds a helpful command to generate new ADRs from that template.

Example: https://asciinema.org/a/56X6g1bA9NpWq0FxW8wDMRJ3g

Closes #1415

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@MSevey MSevey marked this pull request as ready for review April 5, 2023 14:13
@MSevey MSevey self-assigned this Apr 5, 2023
liamsi
liamsi previously approved these changes Apr 5, 2023
rootulp
rootulp previously approved these changes Apr 5, 2023
Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

[non-blocking] I noticed that make adr-gen without any arguments copy + pastes the template into to docs/architecture/adr--.md because the make command assumes those variables to be defined. It would be nice (although totally optional) if the filename was docs/architecutre/adr-template.md if the arguments weren't specified.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
evan-forbes
evan-forbes previously approved these changes Apr 6, 2023
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

utACK +1 to Rootul's comments

Co-authored-by: Rootul P <[email protected]>
@evan-forbes evan-forbes dismissed stale reviews from rootulp, liamsi, and themself via 1d4103a April 17, 2023 12:45
@MSevey MSevey requested a review from a team April 17, 2023 12:45
Co-authored-by: Rootul P <[email protected]>
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

I approved some sensible comments while Matt is OOO so we can push this through

@rootulp rootulp merged commit f5aaf3e into main Apr 17, 2023
@rootulp rootulp deleted the sevey/make-adr branch April 17, 2023 19:39
rootulp added a commit that referenced this pull request Apr 17, 2023
Addresses
#1599 (review)

## Testing

```shell
$ make adr-gen

$ git status
On branch rp/adr-gen
Your branch is up to date with 'origin/rp/adr-gen'.

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	docs/architecture/adr-template.md
```
0xchainlover pushed a commit to celestia-org/celestia-app that referenced this pull request Aug 1, 2024
Addresses
celestiaorg/celestia-app#1599 (review)

## Testing

```shell
$ make adr-gen

$ git status
On branch rp/adr-gen
Your branch is up to date with 'origin/rp/adr-gen'.

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	docs/architecture/adr-template.md
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

celestiaorg ADR template
4 participants