Make units consistent for memory fixes issue #78 #103

mgoodhart5 · 2020-01-16T17:35:34Z

Fixes issue #78
Memory is now returned as Mi for recommendations
All tests passing

claassistantio · 2020-01-16T17:35:39Z

All committers have signed the CLA.

codecov · 2020-01-16T17:46:16Z

Codecov Report

Merging #103 into master will increase coverage by 0.62%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
+ Coverage   42.77%   43.39%   +0.62%     
==========================================
  Files           8        8              
  Lines         547      553       +6     
==========================================
+ Hits          234      240       +6     
  Misses        302      302              
  Partials       11       11

Impacted Files	Coverage Δ
pkg/dashboard/helpers.go	`80.24% <100%> (+1.58%)`	⬆️

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 7aa135d...bdff547. Read the comment docs.

sudermanjr · 2020-01-16T18:44:47Z

pkg/dashboard/helpers.go

 	uuid "github.com/satori/go.uuid"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 )

-func printResource(quant resource.Quantity) string {
+func printResource(quant resource.Quantity, resourceType corev1.ResourceName) string {
+	// weird thing you can't look at


I don't think we need this comment ;-)

sudermanjr · 2020-01-16T18:45:18Z

pkg/dashboard/helpers_test.go

@@ -29,31 +30,43 @@ func Test_getUUID(t *testing.T) {
 }

 func Test_printResource(t *testing.T) {
+	// struct to contain test cases defined below


Unnecessary comments

It took me a long time after graduating to accept this, but comments are considered harmful 😅

rbren · 2020-01-16T18:51:59Z

pkg/dashboard/helpers.go

 	if quant.IsZero() {
 		return "Not Set"
 	}
-	return quant.String()
+	switch resourceType {


Minor nit: I think this switch duplicates quant.String(). This might be simpler:

if resourceType == corev1.ResourceMemory { quant = quant.RoundUp(resource.Mega) } return quant.String()

I am not sure I understand this, but happy to talk about it.

Interesting. We were following the pattern from another example. Will that guarantee that Mi is returned rather than k for the units? I am not familiar with the quant.RoundUp function.

Also, what happens to CPU or other types of values in that case?

Not entirely sure why this doesn't want to work, but this is the error I got when I tried it.
cannot use quant.RoundUp(resource.Mega) (type bool) as type resource.Quantity in assignment go
Something with a type difference?

Decided to just leave this for now. We can optimize later if we want

rbren · 2020-01-16T18:54:09Z

pkg/dashboard/helpers_test.go

@@ -29,31 +30,43 @@ func Test_getUUID(t *testing.T) {
 }

 func Test_printResource(t *testing.T) {
+	// struct to contain test cases defined below


It took me a long time after graduating to accept this, but comments are considered harmful 😅

…airwindsOps#103)" This reverts commit e55026d.

* Revert "Make units consistent for memory fixes issue #78 (#103)" This reverts commit e55026d. * Add FormatResourceList and test cases * Use v1 resource memory constants instead of hardcoded strings Co-authored-by: John Turner <jturner@squarespace.com>

* Revert "Make units consistent for memory fixes issue #78 (#103)" This reverts commit e55026d. * Add FormatResourceList and test cases * Use v1 resource memory constants instead of hardcoded strings * Reorganize Reconciler code ordering The vpa.Reconciler function read a bit funny to me: a bunch of private methods were listed before the exported methods. When someone is going to read that file, they most likely want to see the public API of the struct before they see the private functions that implement it. This commit reorders things so that the public API is listed first in the file. * Add DryRun field to the Reconciler The vpa.Reconciler currently uses a passthrough dry-run variable. However the dry run behavior is part of the state of the reconciler, and it shouldn't change within the lifecycle of the reconciler. Decoupling it from the reconciler gives the appearance that the dry run behavior should somehow be separate, and results in a variable that has to be passed around to private package functions. * Make list/create/deleteVPA Reconciler methods These functions all take data clumps of internal state of the Reconciler. While decoupling them might be useful if they were exported package functions, the fact that they're unexported means that the Reconciler is their only caller. Given those two facts, it makes more sense to couple the these functions with the Reconciler. * Rename checkNamespaceLabel to namespaceIsManaged The name checkNamespaceLabel describes what is done mechanically, not why. namespaceIsManaged should hopefully give a clearer sense of what the boolean return value means for this method. * Refactor Reconciler.ReconcileNamespace The main exported API on of the vpa package is the Reconciler.ReconcilerNamespace method. However, this method is a bit hard to read because it mixes levels of abstraction quite frequently. This leads to comments being used to convey semantic information. This commit refactors the main reconciliation loop into a series of unexported methods, allowing the reader of the code to get a better sense of why certain operations are taking place. Co-authored-by: John Turner <jturner@squarespace.com> Co-authored-by: Andrew Suderman <andrew@sudermanjr.com>

* Revert "Make units consistent for memory fixes issue #78 (#103)" This reverts commit e55026d. * Add FormatResourceList and test cases * Use v1 resource memory constants instead of hardcoded strings * Add a VPAUpdateMode to the vpa.Reconciler The VPAUpdateMode field will allow us to use Goldilocks to create VPAs that operate outside of "off" mode. * Add ability to specify vpa update mode This commit adds a parameter to createVPA that allows the user to specify the update mode for the VPA. The mechanism for specifying this is the label goldilocks.fairwinds.com/vpaUpdateMode. * Extract variables instead of hardcoded values * VPA update mode label case insensitive Kubernetes labels are based on DNS names, which are case insensitive. However, the current label used to specify the update mode of created VPAs, vpaUpdateMode, is case sensitive. This has caused a bug that results in all VPAs being created in "Off" mode. This commit fixes that bug by using a case-insensitive label to specify the vpa-update-mode. * Summary fix for multiple containers in a pod (#123) * go.mod updated for some reason * Add support for all VPA UpdateModes * Use ParseBool for deployment enabled label value * Use ParseBool for namespace enabled label value * Use lowercase for switch statement Co-authored-by: John Turner <jturner@squarespace.com> Co-authored-by: David Noyes <david.noyes@skybettingandgaming.com>

* Revert "Make units consistent for memory fixes issue #78 (#103)" This reverts commit e55026d. * Add FormatResourceList and test cases * Use v1 resource memory constants instead of hardcoded strings * Add a VPAUpdateMode to the vpa.Reconciler The VPAUpdateMode field will allow us to use Goldilocks to create VPAs that operate outside of "off" mode. * Add ability to specify vpa update mode This commit adds a parameter to createVPA that allows the user to specify the update mode for the VPA. The mechanism for specifying this is the label goldilocks.fairwinds.com/vpaUpdateMode. * Extract variables instead of hardcoded values * VPA update mode label case insensitive Kubernetes labels are based on DNS names, which are case insensitive. However, the current label used to specify the update mode of created VPAs, vpaUpdateMode, is case sensitive. This has caused a bug that results in all VPAs being created in "Off" mode. This commit fixes that bug by using a case-insensitive label to specify the vpa-update-mode. * Summary fix for multiple containers in a pod (#123) * go.mod updated for some reason * Add support for all VPA UpdateModes * Use ParseBool for deployment enabled label value * Use ParseBool for namespace enabled label value * Use lowercase for switch statement * Add Deployment Exclusion Annotation Also: - Update reconciler to use VPA and Deployment objects directly (instead of names) - Update reconciler logging - Update/Add vpa tests * Change no-vpa annotation to vpa-opt-out (to opt out of vpa in auto mode) Also: - Update vpa tests - Fix vpa-opt-out logic * Add more logging to VPA reconciler * Fix namespace logs to use Name instead of the full Namespace * Update README * Fix Typo aut -> auto Co-Authored-By: Andrew Suderman <andrew@sudermanjr.com> * Fix grammar matched -> match Co-Authored-By: Andrew Suderman <andrew@sudermanjr.com> * Use vpa-update-mode as an annotation for Deployment specific VPA control * Fix ineffectual assignment * Fix test label * Use index to get correct vpa * Use short-hand if statement for map lookup Co-authored-by: John Turner <jturner@squarespace.com> Co-authored-by: David Noyes <david.noyes@skybettingandgaming.com> Co-authored-by: Andrew Suderman <andrew@sudermanjr.com>

…154) * Revert "Make units consistent for memory fixes issue #78 (#103)" This reverts commit e55026d. * Add FormatResourceList and test cases * Use v1 resource memory constants instead of hardcoded strings * Add a VPAUpdateMode to the vpa.Reconciler The VPAUpdateMode field will allow us to use Goldilocks to create VPAs that operate outside of "off" mode. * Add ability to specify vpa update mode This commit adds a parameter to createVPA that allows the user to specify the update mode for the VPA. The mechanism for specifying this is the label goldilocks.fairwinds.com/vpaUpdateMode. * Extract variables instead of hardcoded values * VPA update mode label case insensitive Kubernetes labels are based on DNS names, which are case insensitive. However, the current label used to specify the update mode of created VPAs, vpaUpdateMode, is case sensitive. This has caused a bug that results in all VPAs being created in "Off" mode. This commit fixes that bug by using a case-insensitive label to specify the vpa-update-mode. * Summary fix for multiple containers in a pod (#123) * go.mod updated for some reason * Add support for all VPA UpdateModes * Use ParseBool for deployment enabled label value * Use ParseBool for namespace enabled label value * Use lowercase for switch statement * Add Deployment Exclusion Annotation Also: - Update reconciler to use VPA and Deployment objects directly (instead of names) - Update reconciler logging - Update/Add vpa tests * Change no-vpa annotation to vpa-opt-out (to opt out of vpa in auto mode) Also: - Update vpa tests - Fix vpa-opt-out logic * Add more logging to VPA reconciler * Fix namespace logs to use Name instead of the full Namespace * Update README * Fix Typo aut -> auto Co-Authored-By: Andrew Suderman <andrew@sudermanjr.com> * Fix grammar matched -> match Co-Authored-By: Andrew Suderman <andrew@sudermanjr.com> * Use vpa-update-mode as an annotation for Deployment specific VPA control * Fix ineffectual assignment * Fix test label * Use index to get correct vpa * Use short-hand if statement for map lookup * Expand the functionality and interface for pkg/summary * Add namespace support for summary command * Standardize Vpa -> VPA * Remove unused cached summary * Fix typo in summary command help * Remove confusing error comment * Revert "Refactored summary pkg and added more tests. (#90)" This reverts commit 998db15. * Remove superfluous v1 import alias * Use var for empty slice initialization Co-Authored-By: Luke Reed <luke@lreed.net> * Report an error when too many namespace arguments are given to the summary command * golint and fixes * Use apps/v1 for summary_test.go * Re-export Option type (accidentally unexported) * Use --namespace flag for summary command instead of args Co-authored-by: John Turner <jturner@squarespace.com> Co-authored-by: David Noyes <david.noyes@skybettingandgaming.com> Co-authored-by: Andrew Suderman <andrew@sudermanjr.com> Co-authored-by: Luke Reed <luke@lreed.net>

* Revert "Make units consistent for memory fixes issue #78 (#103)" This reverts commit e55026d. * Add FormatResourceList and test cases * Use v1 resource memory constants instead of hardcoded strings * Add a VPAUpdateMode to the vpa.Reconciler The VPAUpdateMode field will allow us to use Goldilocks to create VPAs that operate outside of "off" mode. * Add ability to specify vpa update mode This commit adds a parameter to createVPA that allows the user to specify the update mode for the VPA. The mechanism for specifying this is the label goldilocks.fairwinds.com/vpaUpdateMode. * Extract variables instead of hardcoded values * VPA update mode label case insensitive Kubernetes labels are based on DNS names, which are case insensitive. However, the current label used to specify the update mode of created VPAs, vpaUpdateMode, is case sensitive. This has caused a bug that results in all VPAs being created in "Off" mode. This commit fixes that bug by using a case-insensitive label to specify the vpa-update-mode. * Summary fix for multiple containers in a pod (#123) * go.mod updated for some reason * Add support for all VPA UpdateModes * Use ParseBool for deployment enabled label value * Use ParseBool for namespace enabled label value * Use lowercase for switch statement * Add Deployment Exclusion Annotation Also: - Update reconciler to use VPA and Deployment objects directly (instead of names) - Update reconciler logging - Update/Add vpa tests * Change no-vpa annotation to vpa-opt-out (to opt out of vpa in auto mode) Also: - Update vpa tests - Fix vpa-opt-out logic * Add more logging to VPA reconciler * Fix namespace logs to use Name instead of the full Namespace * Update README * Fix Typo aut -> auto Co-Authored-By: Andrew Suderman <andrew@sudermanjr.com> * Fix grammar matched -> match Co-Authored-By: Andrew Suderman <andrew@sudermanjr.com> * Use vpa-update-mode as an annotation for Deployment specific VPA control * Fix ineffectual assignment * Fix test label * Use index to get correct vpa * Use short-hand if statement for map lookup * Split templates by kind * Restructure/Organize pkg/dashboard into sub-packages * Add options pattern to pkg/dashboard * Move handlers pkg back to dashboard ; Move dashboard.go -> router.go * Change Summary format to better represent its logical data structure * Change templates to work with new Summary structure * Fix container resource recommendation formatting * Move pkg/dashboard -> pkg/web * Split templates and dashboard into files ; Make template util functions more flexible for the router and for new webpages * Add /namespaces ; Make /namespaces the default route ; Style /namespaces ; Fix /dashboard * Limit namespace-list.go data passed to template * Fix dashboard link * Add length to All Namespaces * Fix summary_test.go * Fix typo 'reccomendations' -> 'recommendations' * Fix typo 'NamespceList' -> 'NamespaceList' * Add clarifying comment about template data limiting * Add outer structure to Summary * Rename dashboard -> web * Allow goldilocks' web RBAC to list/get namespaces * Move common labels to utils.go * Use correct labels location * Attempt to fix e2e * Fix incorrect args usage for namespace * Add oidc auth support * Remove redundant return statement from health.go * Update cmd/web.go Co-authored-by: Harrison Katz <hkatz@squarespace.com> * Rename web back to -> dashboard * Batch Summary Deployments list into a single API call ; Improves speed _significantly_ * Remove superfluous Error prefix in error message Co-authored-by: Andrew Suderman <andrew@sudermanjr.com> Co-authored-by: John Turner <jturner@squarespace.com> Co-authored-by: David Noyes <david.noyes@skybettingandgaming.com> Co-authored-by: Andrew Suderman <andrew@sudermanjr.com>

* Revert "Make units consistent for memory fixes issue #78 (#103)" This reverts commit e55026d. * Add FormatResourceList and test cases * Use v1 resource memory constants instead of hardcoded strings * Add a VPAUpdateMode to the vpa.Reconciler The VPAUpdateMode field will allow us to use Goldilocks to create VPAs that operate outside of "off" mode. * Add ability to specify vpa update mode This commit adds a parameter to createVPA that allows the user to specify the update mode for the VPA. The mechanism for specifying this is the label goldilocks.fairwinds.com/vpaUpdateMode. * Extract variables instead of hardcoded values * VPA update mode label case insensitive Kubernetes labels are based on DNS names, which are case insensitive. However, the current label used to specify the update mode of created VPAs, vpaUpdateMode, is case sensitive. This has caused a bug that results in all VPAs being created in "Off" mode. This commit fixes that bug by using a case-insensitive label to specify the vpa-update-mode. * Summary fix for multiple containers in a pod (#123) * go.mod updated for some reason * Add support for all VPA UpdateModes * Use ParseBool for deployment enabled label value * Use ParseBool for namespace enabled label value * Use lowercase for switch statement * Add Deployment Exclusion Annotation Also: - Update reconciler to use VPA and Deployment objects directly (instead of names) - Update reconciler logging - Update/Add vpa tests * Change no-vpa annotation to vpa-opt-out (to opt out of vpa in auto mode) Also: - Update vpa tests - Fix vpa-opt-out logic * Add more logging to VPA reconciler * Fix namespace logs to use Name instead of the full Namespace * Update README * Fix Typo aut -> auto Co-Authored-By: Andrew Suderman <andrew@sudermanjr.com> * Fix grammar matched -> match Co-Authored-By: Andrew Suderman <andrew@sudermanjr.com> * Use vpa-update-mode as an annotation for Deployment specific VPA control * Fix ineffectual assignment * Fix test label * Use index to get correct vpa * Use short-hand if statement for map lookup * Split templates by kind * Restructure/Organize pkg/dashboard into sub-packages * Add options pattern to pkg/dashboard * Move handlers pkg back to dashboard ; Move dashboard.go -> router.go * Change Summary format to better represent its logical data structure * Change templates to work with new Summary structure * Fix container resource recommendation formatting * Move pkg/dashboard -> pkg/web * Split templates and dashboard into files ; Make template util functions more flexible for the router and for new webpages * Add /namespaces ; Make /namespaces the default route ; Style /namespaces ; Fix /dashboard * Limit namespace-list.go data passed to template * Fix dashboard link * Add length to All Namespaces * Fix summary_test.go * Fix typo 'reccomendations' -> 'recommendations' * Fix typo 'NamespceList' -> 'NamespaceList' * Add clarifying comment about template data limiting * Add outer structure to Summary * Rename dashboard -> web * Allow goldilocks' web RBAC to list/get namespaces * Move common labels to utils.go * Use correct labels location * Attempt to fix e2e * Fix incorrect args usage for namespace * Add oidc auth support * Remove redundant return statement from health.go * Update cmd/web.go Co-authored-by: Harrison Katz <hkatz@squarespace.com> * Rename web back to -> dashboard * Batch Summary Deployments list into a single API call ; Improves speed _significantly_ * Remove superfluous Error prefix in error message Co-authored-by: Andrew Suderman <andrew@sudermanjr.com> * Add namespace list search bar/filter ; Add some friendly js improvements * Add Namespace to blank Summary when summarizing a specific Namespace * Fix nil map for namespace summary * Only add containers to the summary if they match a container in the deployment * Add label element for accessibility * Remove on enter namespace selection in the javascript for the namespace list page Co-authored-by: John Turner <jturner@squarespace.com> Co-authored-by: David Noyes <david.noyes@skybettingandgaming.com> Co-authored-by: Andrew Suderman <andrew@sudermanjr.com>

* Revert "Make units consistent for memory fixes issue #78 (#103)" This reverts commit e55026d. * Add FormatResourceList and test cases * Use v1 resource memory constants instead of hardcoded strings * Add a VPAUpdateMode to the vpa.Reconciler The VPAUpdateMode field will allow us to use Goldilocks to create VPAs that operate outside of "off" mode. * Add ability to specify vpa update mode This commit adds a parameter to createVPA that allows the user to specify the update mode for the VPA. The mechanism for specifying this is the label goldilocks.fairwinds.com/vpaUpdateMode. * Extract variables instead of hardcoded values * VPA update mode label case insensitive Kubernetes labels are based on DNS names, which are case insensitive. However, the current label used to specify the update mode of created VPAs, vpaUpdateMode, is case sensitive. This has caused a bug that results in all VPAs being created in "Off" mode. This commit fixes that bug by using a case-insensitive label to specify the vpa-update-mode. * Summary fix for multiple containers in a pod (#123) * go.mod updated for some reason * Add support for all VPA UpdateModes * Use ParseBool for deployment enabled label value * Use ParseBool for namespace enabled label value * Use lowercase for switch statement * Add Deployment Exclusion Annotation Also: - Update reconciler to use VPA and Deployment objects directly (instead of names) - Update reconciler logging - Update/Add vpa tests * Change no-vpa annotation to vpa-opt-out (to opt out of vpa in auto mode) Also: - Update vpa tests - Fix vpa-opt-out logic * Add more logging to VPA reconciler * Fix namespace logs to use Name instead of the full Namespace * Update README * Fix Typo aut -> auto Co-Authored-By: Andrew Suderman <andrew@sudermanjr.com> * Fix grammar matched -> match Co-Authored-By: Andrew Suderman <andrew@sudermanjr.com> * Use vpa-update-mode as an annotation for Deployment specific VPA control * Fix ineffectual assignment * Fix test label * Use index to get correct vpa * Use short-hand if statement for map lookup * Split templates by kind * Restructure/Organize pkg/dashboard into sub-packages * Add options pattern to pkg/dashboard * Move handlers pkg back to dashboard ; Move dashboard.go -> router.go * Change Summary format to better represent its logical data structure * Change templates to work with new Summary structure * Fix container resource recommendation formatting * Move pkg/dashboard -> pkg/web * Split templates and dashboard into files ; Make template util functions more flexible for the router and for new webpages * Add /namespaces ; Make /namespaces the default route ; Style /namespaces ; Fix /dashboard * Limit namespace-list.go data passed to template * Fix dashboard link * Add length to All Namespaces * Fix summary_test.go * Fix typo 'reccomendations' -> 'recommendations' * Fix typo 'NamespceList' -> 'NamespaceList' * Add clarifying comment about template data limiting * Add outer structure to Summary * Rename dashboard -> web * Allow goldilocks' web RBAC to list/get namespaces * Move common labels to utils.go * Use correct labels location * Attempt to fix e2e * Fix incorrect args usage for namespace * Add oidc auth support * Remove redundant return statement from health.go * Update cmd/web.go Co-authored-by: Harrison Katz <hkatz@squarespace.com> * Rename web back to -> dashboard * Batch Summary Deployments list into a single API call ; Improves speed _significantly_ * Remove superfluous Error prefix in error message Co-authored-by: Andrew Suderman <andrew@sudermanjr.com> * Add namespace list search bar/filter ; Add some friendly js improvements * Add Namespace to blank Summary when summarizing a specific Namespace * Fix nil map for namespace summary * Only add containers to the summary if they match a container in the deployment * Add label element for accessibility * Remove on enter namespace selection in the javascript for the namespace list page * Pass proper pointer to Namespace through the vpa reconciler * Pass proper pointer to Namespace through the vpa reconciler ; Always update VPAs during reconciliation * Fix pkg/vpa tests * Get desired VPA object based on existing VPA object * Fix updateVPA() to correctly update existing VPAs and retry with backoff * Remove unused updateModeOff variable from pkg/vpa/test_constants.go Co-authored-by: John Turner <jturner@squarespace.com> Co-authored-by: David Noyes <david.noyes@skybettingandgaming.com> Co-authored-by: Andrew Suderman <andrew@sudermanjr.com>

Make units consistent for memory issue #78

fd77203

mgoodhart5 requested review from sudermanjr and dosullivan January 16, 2020 17:35

sudermanjr approved these changes Jan 16, 2020

View reviewed changes

sudermanjr requested a review from rbren January 16, 2020 18:45

rbren approved these changes Jan 16, 2020

View reviewed changes

Removing comments

bdff547

mgoodhart5 merged commit e55026d into master Jan 16, 2020

delete-merged-branch bot deleted the mg/make-units-consistent branch January 16, 2020 22:50

hjkatz pushed a commit to hjkatz/goldilocks that referenced this pull request Feb 28, 2020

Revert "Make units consistent for memory fixes issue FairwindsOps#78 (F…

73f3246

…airwindsOps#103)" This reverts commit e55026d.

hjkatz mentioned this pull request Apr 13, 2020

Hkatz/vpa deployment exclusion and updates (#140) hjkatz/goldilocks#6

Merged

hjkatz mentioned this pull request Aug 12, 2020

Hkatz/batch summary deployments api call (#194) hjkatz/goldilocks#12

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make units consistent for memory fixes issue #78 #103

Make units consistent for memory fixes issue #78 #103

mgoodhart5 commented Jan 16, 2020 •

edited

Loading

claassistantio commented Jan 16, 2020 •

edited

Loading

codecov bot commented Jan 16, 2020 •

edited

Loading

sudermanjr Jan 16, 2020

sudermanjr Jan 16, 2020

rbren Jan 16, 2020

rbren Jan 16, 2020

mgoodhart5 Jan 16, 2020

sudermanjr Jan 16, 2020

sudermanjr Jan 16, 2020

mgoodhart5 Jan 16, 2020

sudermanjr Jan 16, 2020

rbren Jan 16, 2020

Make units consistent for memory fixes issue #78 #103

Make units consistent for memory fixes issue #78 #103

Conversation

mgoodhart5 commented Jan 16, 2020 • edited Loading

claassistantio commented Jan 16, 2020 • edited Loading

codecov bot commented Jan 16, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgoodhart5 commented Jan 16, 2020 •

edited

Loading

claassistantio commented Jan 16, 2020 •

edited

Loading

codecov bot commented Jan 16, 2020 •

edited

Loading