search

m-lab / etl

M-Lab ingestion pipeline
Apache License 2.0
22 stars 7 forks source link

Add parser.GitCommit field into ParseInfo struct #1005

Closed cristinaleonr closed 3 years ago

cristinaleonr commented 3 years ago

Summary: Adding GitCommit field to ParseInfo struct

Description: This struct is used by NDT7 and Annotation NDT5 uses another version called ParseInfoV0, so it's not in scope for this change

Testing: Change includes unit tests and will also be tested by checking the table schema

This change is Reviewable

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 6532

Totals Coverage Status
Change from base Build 6526: 0.2%
Covered Lines: 3492
Relevant Lines: 5570
💛 - Coveralls
cristinaleonr commented 3 years ago

parser/parser.go, line 49 at r2 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…
I know we touched on this conversationally. So, I'm sorry for the mixed message. I think we should include the complete hash here. The NDT result schema includes a field named `GitShortCommit` https://github.com/m-lab/ndt-server/blob/master/data/result.go#L55 that is the short commit. To prevent having two names for the same things, I would suggest that we either use the complete commit hash here, or change the field name to GitShortCommit (and keep the short hash). I recommend the complete hash here.

Apologies, I misunderstood our conversation. I changed it to use the complete hash now.

cristinaleonr commented 3 years ago

parser/annotation_test.go, line 53 at r3 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…
Good addition. Could you modify to test if != 1 and t.Fatal(), and then do the body outside the conditional?

Thanks for the suggestion.

The test cases also include a case for corrupt input (lines 27-29 of annotation_test.go), which fails that check. I could move that case out into a separate function?