-
-
Notifications
You must be signed in to change notification settings - Fork 99
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
ci: add ADR lint and PR validation #713
Conversation
✅ Deploy Preview for modest-rosalind-098b67 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for asyncapi-studio-design-system ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
.github/workflows/lint-pr-adr.yml
Outdated
const title = context.payload.pull_request.title; | ||
const labels = context.payload.pull_request.labels; | ||
const hasAdrLabel = labels.some(label => label.name === 'kind/adr'); | ||
const re = /^chore:\s*\[ADR-(\d{4})\]/; |
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 see this considers ADR having a semantic title of chore
. I think It would be great to drop semantic release titles and instead use changesets for studio. I will create an ADR for that and describe what changes needs to be considered for us. till then, I think we should not merge this PR.
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.
@KhudaDad414 I see your point and make sense, especially if we are planning to drop semantic release titles. However as the PR ADRs have no intent to trigger any release I'm wondering if we couldn't just create a new ADR
repo for the AsyncAPI initiative and invite other projects to use it as well.
Any thoughts folks ?
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 see the value but to be honest I'd not do it yet. We yet have to prove their value of them for us and I'd slowly roll it out on each project instead of doing it at once for the whole initiative. It will be easier for you to convince folks and adapt their workflow if you do it one by one.
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.
@KhudaDad414 what if instead of blocking this PR we make the regex to either support chore: [ADR-NNNN]
or [ADR-NNNN]
as the title? This way we can already unblock this, start using ADRs, and as soon as the migration to changesets is done, we can remove the chore: [ADR-NNNN]
option.
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've seen this in the doc
Since changesets are focused on releases and changelogs, changes to your repository that don't require these won't need a changeset. As such, we recommend not adding a blocking element to contributions in the absence of a changeset.
It means that contributors will only commit the ADR files without the need to bump a release or use changesets ?
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.
Yeah, IMHO ADRs should not trigger a new release and, therefore, it shouldn't contain a changeset.
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.
@fmvilas agreed. let's merge this one.
Please retry analysis of this Pull-Request directly on SonarCloud. |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
@Amzani I have made the changes, please merge when ready. 🙇 |
/rtm |
Description
Fixes #677