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

ADR template for Pull Request #296

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

bjohansebas
Copy link
Member

Related #291

@bjohansebas bjohansebas requested review from UlisesGascon and a team October 28, 2024 21:38
Copy link
Member

@wesleytodd wesleytodd left a comment

Choose a reason for hiding this comment

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

I put request changes, but I think it is not truly something I consider blocking. I am just worried folks will be like me and need to go look up what "ADR" means to understand.

.github/PULL_REQUEST_TEMPLATE/adr-template.md Outdated Show resolved Hide resolved
@@ -0,0 +1,77 @@
---
name: ADR Template
Copy link
Member

Choose a reason for hiding this comment

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

Since I was not aware of the ADR format, I wonder if we could use more commonly used terminology here? RFC is the one I find most common and I know @sheplu even mentioned that on our call. That said, "proposal" would also make sense to me, and I think is more in the loose spirit of what these can be.

Copy link
Member

Choose a reason for hiding this comment

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

RFC sounds great! :)

Copy link
Member

Choose a reason for hiding this comment

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

an RFC is something different and I think we should prefer the semantically correct term for what this represents. RFC !== ADR

@bjohansebas
Copy link
Member Author

I was thinking about this PR, and I don't think it's best to include all the content of the ADR as the message of the PR, as it would just duplicate the content that the PR would carry. Perhaps we should provide a message stating 'use the template located in X place' and add a section in the README educating about the ADRs. What do you think?

@UlisesGascon
Copy link
Member

I think that is good idea @bjohansebas, so we only make changes in one place too.

@jonchurch
Copy link
Member

jonchurch commented Nov 2, 2024

I don't see the need for a PR template for this.

Functionally, unless it is the default PR template (named pull_request_template.md), the only way to open a PR w/ this template is via querystrings specifying the template name and visiting that link. People create workarounds for that but they're clunky.

The value I do see here is adding an ADR label. We can do that via an action like:
(PR #300)

name: ADR Labeler

on:
  pull_request:
    types: [opened, synchronize]
    paths:
      - 'docs/adr/**'

jobs:
  label-adr:
    runs-on: ubuntu-latest
    permissions:
      pull-requests: write
    
    steps:
      - uses: actions/github-script@v7
        with:
          script: |
            # Adding ADR label - if it already exists, this is a no-op
            github.rest.issues.addLabels({
              owner: context.repo.owner,
              repo: context.repo.repo,
              issue_number: context.issue.number,
              labels: ['ADR']
            })

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.

6 participants