-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Implement validation for PR titles based on Conventional Commits and Jira references #11
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
base: main
Are you sure you want to change the base?
Conversation
…s and Jira references - removes dependency on external action - uses deno - unit tests our validate PR title logic via Deno - integration tests via act with mock payload - renames workflow to be more purpose oriented
|
|
| desc: "accepts revert with scope and ticket", | ||
| }, | ||
| { | ||
| title: "feat(api): SC-1234-description-with-hyphens", |
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 disagree that this should pass!
The changelog should be readable not some identifier.
This is also why I'm in favor of putting the ticket at the end, if you read the changelog the least important information is the ticket.
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 discussed briefly in backend check-in -> we don't care strongly about this -> i'll put it to the end
|
i'm mainly not sure on this functionality, if it will work with this PR too: |
|
|
||
| const invalidCases = [ | ||
| { | ||
| title: "feat: Add feature", |
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 also think we should allow no tickets.
There are simply changes where creating a ticket is not useful.
The most important example: Deploying a domain 👀 There we also don't want to have a ticket every time.
Though other changes like updating docs, READMEs, etc. would be too annoying to create an issue just to have an issue.
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.
yes fair point -> i'll make it warn, but otherwise succeed
|
In general, I'd hold of from making such a dramatic change to the workflow right now because the entire premise of this running like this is in question due to limitations of GitHub. |
|
moving @IchordeDionysos comments here:
for some basic linting of one sentence, I struggle to see the justification of any package for it, for something that would be used across all of our repos - it reminds me of this: https://en.wikipedia.org/wiki/Npm_left-pad_incident Sure, we could mitigate it by pinning.. but it's even less risk if we don't have any package! |
mainly addressing my own comments: #10 (review)
i went a bit back and forth on whether to implement just via bash vs javascript, but doing unit tests on scripts in bash felt yucky
https://www.loom.com/share/38f47335440649b5ad54ca9a15ede04b
https://simpleclub.slack.com/archives/C0123GV10US/p1764342932709379?thread_ts=1764342600.513229&cid=C0123GV10US
note this comment:
I’m holding off making changes until I get feedback from GitHub support on why the setup doesn’t work …-> maybe we can't merge this until then... or maybe that's exactly why we should merge it... not sure!