ci: Add conventional commit PR check #1926
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @mikkokotila)
a discussion (no related file):
Did you evaluate different actions? What signals did you use to determine that of the available actions, that this is of good quality and is well-maintained? Did you consider the alternatives such as wrapping CommitLint with our own action and weigh that against the off-the-shelf options?
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @mikkokotila)
a discussion (no related file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Did you evaluate different actions? What signals did you use to determine that of the available actions, that this is of good quality and is well-maintained? Did you consider the alternatives such as wrapping CommitLint with our own action and weigh that against the off-the-shelf options?
Is the action you chose as configurable as CommitLint? We probably want to start with the Angular configuration as a base, but use slightly different rules such as allowing the message itself to start with an uppercase letter, etc.
No, none of that. I simply tested it in a another repo, and ran through few common scenarios to test it. It is a very simple action, which is very easy to replace if need to. In other words, risk of negative consequences arising from adoption are very low and easy to mitigate in case of arising. |
There was a problem hiding this comment.
That is not something you can just simply state. It is below the standards of rigor for this project. For example, from our Code Style:
Limit usage of unnecessary third party dependencies. Due to the privacy requirements of this project, we prefer avoiding unaudited dependencies.
While this is a process dependency and not a library dependency and therefore less susceptible to risk, we still always evaluate third-party dependencies. More broadly, every change on this project is expected to have gone through the appropriate level of rigor in terms of considering alternatives. For example, this is the purpose of the Caveats section in the design doc template.
As an aside, it appears that you replied to the comment directly on GitHub rather than using Reviewable as mentioned in the Dev Standards doc
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @mikkokotila)
There was a problem hiding this comment.
we still always evaluate third-party dependencies
Do you mean that this level of step requires completing the design doc template? So, using a more typical open source way of saying it, if one wants to introduce a new workflow action, one must make an RFC for it first?
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @kungfucraig and @SanjayVas)
a discussion (no related file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Is the action you chose as configurable as CommitLint? We probably want to start with the Angular configuration as a base, but use slightly different rules such as allowing the message itself to start with an uppercase letter, etc.
Sure, if we want to take an alternative route, there is no need to put more time into this here. But I would strongly advise against making it more flexible. There is great value in adopting a common way of working, like Conventional Commits, and adhering to it. Especially things like allowing flexible capitalization should be avoided, as it does not offer any value but has the important downside of lack of consistency.
|
Hey @mikkokotila, I think the way to go about this would be to write the "RFC", aka one-pager, that outlines one or more options for this. Then circulate that on slack and we can discuss at our eng review meeting in a week and decide on something. I think that will end up being more efficient than designing within the scope of a PR. |
Closes #1908