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.

Sign up for GitHub

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

Jump to bottom

build: add release as a valid commit message subject #19955

Closed
wants to merge 2 commits into from
Closed

build: add release as a valid commit message subject #19955

wants to merge 2 commits into from

Conversation

matsko
Copy link
Contributor

@matsko matsko commented Oct 26, 2017 •
edited

This should make quiet down the commit message CircleCI issues.

@matsko matsko requested review from alexeagle and alxhub and removed request for alexeagle October 26, 2017 18:00
@gkalpak
Copy link
Member

gkalpak commented Oct 26, 2017 •
edited

It will silence CircleCI (for now 😃), but is a work-around, not a proper fix 🤓
The reason validate-commit-message complains for a commit that is not part of the PR is tries to validate, is that it calculates the commit range incorrectly.

Background:
package.json#version is being used by validate-commit-message to determine the branch a PR is submitted against. Right now it thinks PRs for master are actually targeting the 5.0.x branch and it tries to validate more commits than necessary (and fails due to the bad release: ... commit).

One possible fix (now that there is a 5.0.x branch) is to bump package.json#version to 5.1.<something>.

(If for some reason it is not desirable to do so until a 5.1.x version is released, we need to find a different way to identify a PR's target branch.)

gkalpak
gkalpak reviewed Oct 26, 2017
View reviewed changes
tools/validate-commit-message/commit-message.json Outdated
@@ -28,6 +28,7 @@
"platform-server",
"platform-webworker",
"platform-webworker-dynamic",
"release",
Copy link
Member

Choose a reason for hiding this comment

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

If you really want to add this as valid commit message subject, you must update the corresponding tests as well.

Copy link
Member

Choose a reason for hiding this comment

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

And CONTRIBUTORS.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I don't think we should make this a known subject because nobody external will do anything relative to a release.

Copy link
Contributor

Choose a reason for hiding this comment

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

@matsko this is the wrong place to put this. you need to update the types array.

and @alxhub and @gkalpak are right, you will need to update the tests and document this even if it's not relevant for external developers. It will be relevant for anyone new joining the team and reading that doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Damn I feel dumb. OK fixed.

alxhub
alxhub approved these changes Oct 26, 2017
View reviewed changes
tools/validate-commit-message/commit-message.json Outdated
@@ -28,6 +28,7 @@
"platform-server",
"platform-webworker",
"platform-webworker-dynamic",
"release",
Copy link
Member

Choose a reason for hiding this comment

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

And CONTRIBUTORS.md

@matsko
Copy link
Contributor Author

matsko commented Oct 27, 2017

@gkalpak let's add the release subject first (in this PR) and then address the next fix on Monday.

@IgorMinar
Copy link
Contributor

all tests must be updated to contain "release" otherwise you'll have CI failures.

IgorMinar
IgorMinar approved these changes Oct 29, 2017
View reviewed changes
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

please clean up this PR per feedback. otherwise lgtm! thanks for making this change that went unnoticed for so long.

tools/validate-commit-message/commit-message.json Outdated
@@ -28,6 +28,7 @@
"platform-server",
"platform-webworker",
"platform-webworker-dynamic",
"release",
Copy link
Contributor

Choose a reason for hiding this comment

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

@matsko this is the wrong place to put this. you need to update the types array.

and @alxhub and @gkalpak are right, you will need to update the tests and document this even if it's not relevant for external developers. It will be relevant for anyone new joining the team and reading that doc.

@IgorMinar IgorMinar added area: build & ci Related the build and CI infrastructure of the project action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Oct 29, 2017
@gkalpak gkalpak mentioned this pull request Oct 30, 2017
Closed
gkalpak
gkalpak reviewed Oct 30, 2017
View reviewed changes
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

Still need to add release as a valid type in CONTRIBUTORS.md.

@matsko
build: temporarily increase the commit msg limit to 120
45d6c02 
Right now HEAD and 5.0.x have a branch deviation and therefore
all the commits between both branches are being compared. There
exists a problematic commit which has a commit message that is
longer than 100 commits. This patch will temporarily increase
the limit to 120 so that CI passes. Once master is resumed to
being the primary development branch (once 5.0.0 is out) then
the the msg limit will be set back to 100.
@matsko matsko added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Oct 31, 2017
matsko added a commit that referenced this pull request Oct 31, 2017
@matsko
build: add release as a valid commit message subject (#19955)
c295aee 
PR Close #19955
@matsko matsko closed this in 951bd33 Oct 31, 2017
@matsko matsko deleted the add_release branch January 17, 2019 19:43
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gkalpak gkalpak gkalpak left review comments

@IgorMinar IgorMinar IgorMinar approved these changes

@hansl hansl hansl approved these changes

@alxhub alxhub alxhub approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: build & ci Related the build and CI infrastructure of the project cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@matsko @gkalpak @IgorMinar @hansl @alxhub @googlebot