Skip to content

Conversation

@ystefinko
Copy link
Collaborator

@ystefinko ystefinko commented Mar 17, 2020

Add script to separate Azure job which verify
correctness of agreed commit message.
Title is less than 50 chars.
Next to title must be clear line.
After that all lines length is less than 80chars.
At the end should be Jira reference.

Resolves: OLPEDGE-1573

Signed-off-by: Yaroslav Stefinko ext-yaroslav.stefinko@here.com

@ystefinko ystefinko added the CI CI setup or improvement related label Mar 17, 2020
@ystefinko ystefinko force-pushed the task/olpedge-1573-ci branch 2 times, most recently from 6622a97 to e8fb1c9 Compare March 17, 2020 13:41
@codecov
Copy link

codecov bot commented Mar 17, 2020

Codecov Report

Merging #721 into master will increase coverage by 0.0%.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #721   +/-   ##
======================================
  Coverage    77.9%   78.0%           
======================================
  Files         286     286           
  Lines        9362    9370    +8     
======================================
+ Hits         7297    7306    +9     
+ Misses       2065    2064    -1     
Impacted Files Coverage Δ
...cpp-sdk-core/include/olp/core/geo/tiling/TileKey.h 100.0% <0.0%> (ø)
...nclude/olp/dataservice/read/PrefetchTilesRequest.h 96.7% <0.0%> (ø)
...-read/src/repositories/PrefetchTilesRepository.cpp 90.7% <0.0%> (+0.2%) ⬆️
...-dataservice-read/src/VersionedLayerClientImpl.cpp 92.3% <0.0%> (+0.3%) ⬆️
...-sdk-dataservice-read/src/VersionedLayerClient.cpp 93.5% <0.0%> (+0.4%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3ead20...403f231. Read the comment docs.

@ystefinko ystefinko force-pushed the task/olpedge-1573-ci branch 2 times, most recently from 739b1bc to 6ef37f0 Compare March 17, 2020 13:52
@ystefinko ystefinko force-pushed the task/olpedge-1573-ci branch 4 times, most recently from 21be38f to f2cfaf1 Compare March 18, 2020 08:13
Copy link
Contributor

Choose a reason for hiding this comment

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

Why -3? These are the last 3 commits.
Actually Travis provides you this already in the env variables -> https://docs.travis-ci.com/user/environment-variables/#default-environment-variables

Actually you have multiple variables that help you:

  • TRAVIS_COMMIT: The commit that the current build is testing.
  • TRAVIS_COMMIT_MESSAGE: The commit subject and body, unwrapped.
  • TRAVIS_COMMIT_RANGE: The range of commits that were included in the push or pull request. (Note that this is empty for builds triggered by the initial commit of a new branch.)

Also git log --pretty=format:'%B' -1 gives you only the body of the commit message.

Copy link
Contributor

Choose a reason for hiding this comment

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

These rules should be tested:

  1. Summary (1st line) is less the 50 chars wide.
  2. Extra newline between summary and detailed text.
  3. The detailed text has more than one line.
  4. Each line of detailed text is less than 72-80 chars.
  5. An extra new line between detailed text and the next section (bullet points or ticket number)
  6. Each commit contains a ticket number and one of "Resolves: " || "Relates-to: " (note the extra space after ":".
  7. If multiple tickets are referenced they should be separated by "," and a space,
    e.g. Resolves: OLPEDGE-123, OLPEDGE-124

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

met all rules except 5 and 7 as not mandatory for now

Copy link
Contributor

@r0busta r0busta Mar 25, 2020

Choose a reason for hiding this comment

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

Fifty chars for the subject is not enough. It doesn't fit a reasonably long sentence as Use OnlineIfNotFound to create the cache in integration tests.

I suggest keeping the standard maximum of eighty chars. (In the check script and the example.)

@ystefinko ystefinko force-pushed the task/olpedge-1573-ci branch 18 times, most recently from 33fe76f to 001e5be Compare March 20, 2020 12:37
@ystefinko ystefinko force-pushed the task/olpedge-1573-ci branch from 001e5be to 678c6e3 Compare March 20, 2020 12:39
@ystefinko ystefinko force-pushed the task/olpedge-1573-ci branch 4 times, most recently from 80ea5bd to b475337 Compare March 23, 2020 09:57
@ystefinko
Copy link
Collaborator Author

ready for review.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why {1..15}?
What if we have more lines.
I would iterate trough the lines from commit.log.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

Why -2? This gives you the last two commits.
Also, I am not sure if this will work for multi commits PRs. Maybe Azure provides some env variable for this, to get all commits, resp to get the oldest commit in the branch as that one should have the proper commit message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

-2 is needed specially for CI like Azure or Travis. Locally you might use -1.
Investigation of needed variables failed due to lack of them and/or inability to use them for now for our VMs, so we should use our own git log command.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please in addition to this error also print the full recommendation always:

Summarize changes in around 50 characters or less.
   
More detailed explanatory text. Wrap it to about 72
characters or so. In some contexts, the first line is treated as the
subject of the commit and the rest of the text as the body. The
blank line separating the summary from the body is critical (unless
you omit the body entirely); various tools like `log`, `shortlog`
and `rebase` can get confused if you run the two together.
   
Explain the problem that this commit is solving. Focus on why you
are making this change as opposed to how (the code explains that).
Are there side effects or other unintuitive consequences of this
change? Here's the place to explain them.
   
Further paragraphs come after blank lines.
   
- Bullet points are okay, too.
   
- Typically a hyphen or asterisk is used for the bullet, preceded
  by a single space, with blank lines in between, but conventions
  vary here.
   
All commits need to reference a ticket, again separated by a blank
line from the commit message summary above:
 
Resolves: ABC-111, #123
Relates-To: ABC-333, #321
See also: ABC-432, #456, #789
 
Signed-off-by: FirstName LastName <firstname.lastname@here.com>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed.Added print of file

@ystefinko ystefinko force-pushed the task/olpedge-1573-ci branch 2 times, most recently from 9b1bac7 to 5e8893f Compare March 24, 2020 09:42
@ystefinko ystefinko requested a review from andescu March 24, 2020 10:11
@ystefinko ystefinko force-pushed the task/olpedge-1573-ci branch 4 times, most recently from dd57c12 to 107f331 Compare March 24, 2020 22:17
andescu
andescu previously approved these changes Mar 25, 2020
Copy link
Contributor

@r0busta r0busta Mar 25, 2020

Choose a reason for hiding this comment

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

50 is too small. Consider this sentence:
Specify the OLP SDK version in the CMake file
it's 45 chars long. Adding a few more words would fail the check.

I suggest keeping the standard maximum of eighty chars

Copy link
Contributor

@r0busta r0busta Mar 25, 2020

Choose a reason for hiding this comment

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

Fifty chars for the subject is not enough. It doesn't fit a reasonably long sentence as Use OnlineIfNotFound to create the cache in integration tests.

I suggest keeping the standard maximum of eighty chars. (In the check script and the example.)

Add script to separate Azure job which verify
correctness of agreed commit message.
Title is less than 50 chars.
Next to title must be clear line.
After that all lines length is less than 72 chars.
At the end should be Jira reference.

Resolves: OLPEDGE-1573

Signed-off-by: Yaroslav Stefinko <ext-yaroslav.stefinko@here.com>
@r0busta
Copy link
Contributor

r0busta commented Mar 25, 2020

As discussed offline; we keep the limits as they are and re-evaluate it in a fortnight.

@ystefinko ystefinko merged commit d22e85b into master Mar 25, 2020
@ystefinko ystefinko deleted the task/olpedge-1573-ci branch March 25, 2020 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI CI setup or improvement related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants