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

Update sign-off location instructions #2096

Closed
wants to merge 1 commit into from
Closed

Update sign-off location instructions #2096

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,9 @@ Fixes: #1234
Sign off on your commit in the footer. By doing this, you assert original
authorship of the commit and that you are permitted to contribute it. This can
be automatically added to your commit by passing `-s` to `git commit`, or by
manually adding the following line to the footer of the commit.
manually adding the following line to the footer of the commit. The footer must
be separated from the preceeding body or header with a blank line, and there must
not be a blank line following the footer.
Copy link
Member

Choose a reason for hiding this comment

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

and there must not be a blank line following the footer. Isn't this covered by the text a few lines down?

Remember, if a blank line is found anywhere after the Signed-off-by line, the
Signed-off-by: will be considered outside of the footer, and will fail the
automated Signed-off-by validation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, just reinforces the previous sentence. I wanted to keep it concise and simple. I'm actually in favor of removing the "Remember, if a blank..." paragraph as it's to verbose. Thoughts?


```
Signed-off-by: Full Name <email>
Expand Down Expand Up @@ -124,6 +126,7 @@ The guidelines are changed to:
message.

Closes: #124

Copy link
Member

Choose a reason for hiding this comment

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

I don't believe this is necessary - both Closes and Signed off can be part of the footer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#1636 (comment) seems to imply otherwise. @IBMJimmyk can you validate the correctness of your comment?

Copy link
Contributor Author

@fjeremic fjeremic Jun 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Actually @DanHeidinga there is a clear example of the check failing in #2098. You can see the failure output (https://ci.eclipse.org/openj9/job/PullRequest-SignedOffByCheck-OpenJDK10/244/):

13:32:51 ###################################
[Pipeline] echo
13:32:55 FAILURE - The following commits appear to have an incorrect sign-off
[Pipeline] echo
13:33:02 commit afc2c20e928648a6277b2d17f5eebd01b2e75699
13:33:02 Author: Daniel Hong <daniel.hong@live.com>
13:33:02 Date:   Thu May 31 09:55:11 2018 -0400
13:33:02 
13:33:02     Re-enable toLowerCase/toUpperCase acceleration
13:33:02     
13:33:02     Modify API call so that caseConversionHelper recieves
13:33:02     underlying arrays and length of string.
13:33:02     
13:33:02     Closes: #1934
13:33:02     Signed-off-by: Daniel Hong <daniel.hong@live.com>
[Pipeline] echo
13:33:05 ###################################

So we do in fact need a new line between the "Closes" and "Signed-off-by" tags in the footer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note the PR referenced has since been fixed up but I've linked the failing job in the above comments. It's easily reproducible 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.

That just means the check we are implementing hasn't been implemented correctly. Its not the Eclipse ip-validation which was failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So do we need an issue against our own check to handle this case and close this PR?

Copy link
Member

Choose a reason for hiding this comment

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

There already is an issue open, the check is WIP and being testing on the OpenJ9 repo.
ibmruntimes/openj9-openjdk-jdk9#58 (comment)
#1941

Yes I think we can close this PR.

Signed-off-by: Robert Young <rwy0717@gmail.com>
```

Expand Down