Skip to content

Conversation

stephen-soltesz
Copy link
Contributor

@stephen-soltesz stephen-soltesz commented Feb 24, 2020

This change forks the annotation-service/api types to allow:

  • differentiated ServerAnnotation and ClientAnnotation types
  • complete Geo2 Subdivision fields in the Geolocation type
  • preserve backward compatible support for parsers using annotation-service/api types while letting the uuid-annotator archive files using canonical camel case field names.

This change also temporarily disables the asn annotator support because the type changes require a different implementation of the route view parsing and search routines. That will be added in a subsequent PR to reenable ASN annotation.


This change is Reviewable

@coveralls
Copy link
Collaborator

coveralls commented Feb 24, 2020

Pull Request Test Coverage Report for Build 121

  • 60 of 63 (95.24%) changed or added relevant lines in 4 files are covered.
  • 84 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-20.7%) to 77.749%

Changes Missing Coverage Covered Lines Changed/Added Lines %
asnannotator/routeview.go 0 3 0.0%
Files with Coverage Reduction New Missed Lines %
asnannotator/routeview.go 84 0%
Totals Coverage Status
Change from base Build 104: -20.7%
Covered Lines: 304
Relevant Lines: 391

💛 - Coveralls

Copy link

@gfr10598 gfr10598 left a comment

Choose a reason for hiding this comment

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

:lgtm: with two questions.

Reviewed 3 of 8 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @gfr10598 and @stephen-soltesz)


asnannotator/routeview.go, line 63 at r1 (raw file):

	}

	// TODO: remove after enabling asn annotations again.

Is it practical to do this now? This code is very confusing as is.


geoannotator/ipannotator.go, line 32 at r1 (raw file):

}

// Annotate puts into geolocation data and ASN data into the passed-in annotations map.

Typo in comment?

Copy link
Contributor Author

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @gfr10598)


asnannotator/routeview.go, line 63 at r1 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

Is it practical to do this now? This code is very confusing as is.

I did this just to make the PR size manageable. The next PR will re-enable this logic.


geoannotator/ipannotator.go, line 32 at r1 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

Typo in comment?

Done.

@stephen-soltesz stephen-soltesz merged commit 733ea46 into master Feb 24, 2020
@stephen-soltesz
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants