feat/ota-4174/direct ostree update of IP Secondary #1500

mike-sul · 2019-12-23T11:28:05Z

Initial version of an ostree direct update support for IP Secondary
Decouple the secondary interface implementation (aktualizr_secondary) from the TCP server
Decouple the secondary implementation common functionality from an update/install method specific (update agent)
Encapsulate the manifest management functionality in a single component and reuse it for Primary and Secondary
Remove some outdated tests for the secondary and add more relevant tests

codecov-io · 2019-12-30T12:28:28Z

Codecov Report

Merging #1500 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1500   +/-   ##
=======================================
  Coverage   80.78%   80.78%           
=======================================
  Files         184      184           
  Lines       11182    11182           
=======================================
  Hits         9033     9033           
  Misses       2149     2149

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 4c41de8...0a74dda. Read the comment docs.

lgtm-com · 2019-12-30T13:07:47Z

This pull request fixes 1 alert when merging 8828cf4 into 2403384 - view on LGTM.com

fixed alerts:

1 for FIXME comment

lgtm-com · 2019-12-31T07:23:27Z

This pull request introduces 1 alert and fixes 1 when merging b404658 into 2403384 - view on LGTM.com

new alerts:

1 for FIXME comment

fixed alerts:

1 for FIXME comment

lgtm-com · 2019-12-31T15:49:16Z

This pull request introduces 1 alert and fixes 1 when merging 6fbbcc0 into 2403384 - view on LGTM.com

new alerts:

1 for FIXME comment

fixed alerts:

1 for FIXME comment

lgtm-com · 2019-12-31T16:42:39Z

This pull request fixes 1 alert when merging 78752fd into 2403384 - view on LGTM.com

fixed alerts:

1 for FIXME comment

lgtm-com · 2019-12-31T21:58:55Z

This pull request fixes 1 alert when merging 4548419 into 2403384 - view on LGTM.com

fixed alerts:

1 for FIXME comment

pattivacek

This is where I've gotten so far, more to come soon! So far, good at a high level, just some small details.

src/aktualizr_secondary/CMakeLists.txt

pattivacek · 2020-01-02T08:37:47Z

src/aktualizr_secondary/aktualizr_secondary.cc

-void AktualizrSecondary::stop() { /* TODO? */
+  if (rebootDetected()) {
+    LOG_INFO << "Reboot has been detected, applying the new ostree revision: " << pending_target_.sha256Hash();
+    // TODO: refactor this


To achieve what goal?

// TODO: refactor this to make it simpler as we don't need to persist/store
// an installation status of each ECU but store it just for a given secondary ECU.
We can get rid of dependency on the sqlstorage at all.

// TODO: refactor this to make it simpler as we don't need to persist/store
// an installation status of each ECU but store it just for a given secondary ECU.

Yep, that makes sense.

We can get rid of dependency on the sqlstorage at all.

How else would you store it, though?

e.g. just a file or a simpler implementation that uses sqlite db

If we're using libaktualizr, we might as well use the sqlite db, but you might be right about using a simpler implementation for this functionality.

src/aktualizr_secondary/aktualizr_secondary.cc

src/aktualizr_secondary/aktualizr_secondary_config_test.cc

src/aktualizr_secondary/aktualizr_secondary_ostree_test.cc

pattivacek

This PR is way, way too big. There is a lot of good stuff here, but it's almost impossible to review, because there are too many things going on at once. This certainly could have been broken down into smaller chunks. At a minimum, the individual refactorings should be done in independent commits. As it is, it's extremely challenging to figure out what changes are related to which other changes. I've done a first pass over this, so we may as well keep it as is, but in the future, we cannot merge PRs this large. They will be rejected.

src/aktualizr_secondary/aktualizr_secondary_factory.h

src/aktualizr_secondary/aktualizr_secondary_test.cc

src/aktualizr_secondary/update_agent_file.cc

pattivacek · 2020-01-02T10:29:01Z

src/aktualizr_secondary/update_agent_ostree.cc

+  try {
+    std::string ca, cert, pkey, server_url;
+    extractCredentialsArchive(data, &ca, &cert, &pkey, &server_url);
+    // TODO: why are qe loading this credentials at all ?


Authentication with Treehub. See OstreeManager::addRemote().

I am not questioning the need in ca/cert/pkey, I don't understand why, at first, we put them into the key manager and then the ostree packman fetch them from the key manager, why not just pass ca/cert/pkey to the packman.

Fair point. My guess is that it either came from mimicking the style of the Primary or that it's only done because it's a hack. If the Secondary is just reusing the same credentials as the Primary, it shouldn't need them at all if the Primary is doing the fetching and proxying the objects to the Secondary. But I know we aren't quite there yet.

Yeah, another item to improve implementation, in the future, but it's a minor at the moment, IMHO.

pattivacek · 2020-01-02T10:34:43Z

src/libaktualizr/crypto/keymanager.cc

@@ -231,6 +232,7 @@ Json::Value KeyManager::signTuf(const Json::Value &in_data) const {
  }

  Json::Value signature;
+  // TODO: FIX the hardcoded value of a signature method/algorithm


@lbonn do I recall that you said this was correct because it is still used even for ED25519? Or what was the story there? I'd like to clear this up.

I'll double check this one too.

Hm I don't recall this particular discussion but it looks wrong in this case, if uptane.key_type is set to ed25519 for example.

Let's not worry about that for this PR; there's already enough going on here. I made a ticket to track this, though: https://saeljira.it.here.com/browse/OTA-4276.

src/libaktualizr/uptane/tuf.cc

src/libaktualizr/uptane/uptanerepository.h

pattivacek · 2020-01-02T11:16:34Z

src/virtual_secondary/virtualsecondary.cc

+  // TODO: it does not make much sense to read, pass via a function parameter to Virtual secondary
+  // and store the file that has been already downloaded by Primary
+  // Primary should apply ECU (primary, virtual, secondary) specific verification, download and installation logic in
+  // the first place


Not sure I agree with this.

Why ? What's your argument(s) ?
I think, it make sense to download a file straight to a desired location/place or copy it from the primary place to the desired location eliminating passing a file content as a string parameter of a function/method call.

I think, it make sense to download a file straight to a desired location/place or copy it from the primary place to the desired location eliminating passing a file content as a string parameter of a function/method call.

I agree with that statement. We shouldn't really be passing strings around like this. It should either be a file handle or a read handle to the storage. Do we have a ticket for that already? We should if we don't.

Primary should apply ECU (primary, virtual, secondary) specific verification, download and installation logic in the first place

This is what I don't agree with. Specifically, the Primary cannot apply installation logic for a Secondary. Or what do you mean by "installation logic", actually? Perhaps I've misunderstood.

Do we have a ticket for that already? We should if we don't.

No, we don't a have a ticket specifically for this issue, but it could be part of the epic on the secondary and package manager refactoring as it most likely will require the secondary interface change.

Sure. Can you make that ticket?

We successfully changed the API call for OpenStoredTarget to use StorageTargetRHandle, which it seems like we might be able do for virtual Secondaries. It probably won't be that simple for "real" Secondaries, though.

API/interface for real and virtual secondaries should be the same, effectively one of the questions here how to turn sendFirmware(const std::string& data) into something more flexible and robust so we can pass a file handler/name in one case, TLS certs and treehub in another and actual image data in some other...

tests/aktualizr.supp

tests/test_fixtures.py

lgtm-com · 2020-01-02T20:53:02Z

This pull request fixes 1 alert when merging d5af374 into e871cc7 - view on LGTM.com

fixed alerts:

1 for FIXME comment

lgtm-com · 2020-01-03T11:07:10Z

This pull request fixes 1 alert when merging 1a9be9a into 3eeccc7 - view on LGTM.com

fixed alerts:

1 for FIXME comment

lgtm-com · 2020-01-03T12:01:55Z

This pull request fixes 1 alert when merging 1446fbb into a6ee6b4 - view on LGTM.com

fixed alerts:

1 for FIXME comment

pattivacek · 2020-01-09T15:50:18Z

I just tested this manually and got this when I tried to provision:

response http code: 400
response: {"code":"invalid_entity","description":"com.advancedtelematic.libats.codecs.DeserializationException: com.advancedtelematic.libats.codecs.RefinementError: Predicate failed: TargetFilename cannot be empty or bigger than 254 chars or contain `..`.\nCaused by: com.advancedtelematic.libats.codecs.RefinementError: Predicate failed: TargetFilename cannot be empty or bigger than 254 chars or contain `..`.\n","cause":null,"errorId":"babbdf48-5e18-4000-a587-06704ad3c339"}
Put manifest request failed: 0  HTTP 400
Error sending manifest!

lgtm-com · 2020-01-13T11:16:11Z

This pull request fixes 1 alert when merging 0f8a6c1 into 45f5416 - view on LGTM.com

fixed alerts:

1 for FIXME comment

lgtm-com · 2020-01-13T12:22:03Z

This pull request fixes 1 alert when merging 1a34721 into 45f5416 - view on LGTM.com

fixed alerts:

1 for FIXME comment

pattivacek

Checkpointing my re-review.

pattivacek · 2020-01-13T12:55:40Z

src/libaktualizr/uptane/imagesrepository.h


+ private:


Redundant private, but not critical at all.

pattivacek · 2020-01-13T13:07:51Z

src/aktualizr_secondary/update_agent_file.cc

+    // TODO: fake package manager
+    const std::string fake_pacman_case = "fake_pacman";
+    installed_image_info.len = fake_pacman_case.size();
+    installed_image_info.hash = Uptane::ManifestIssuer::generateVersionHashStr(fake_pacman_case);


I'd probably prefer to leave the hash empty, although I'm open to discussion on what the right thing to do is. Either way, not enough to hold this up for now.

The backend will reject a manifest with an empty hash value.

pattivacek · 2020-01-13T13:08:47Z

src/aktualizr_secondary/update_agent_ostree.cc

+
+#include "package_manager/ostreemanager.h"
+
+// TBD: consider moving this and SotaUptaneClient::secondaryTreehubCredentials() to encapsulate them in one place that


Any reason to chose "TBD" over "TODO"? It makes grepping more challenging, unless that was the goal.

will replace it with TODO.

pattivacek · 2020-01-13T13:11:01Z

src/libaktualizr-posix/ipuptanesecondary.h


-  static std::pair<bool, std::shared_ptr<Uptane::SecondaryInterface>> create(const std::string& address,
-                                                                             unsigned short port, int con_fd);
+  static SecondaryInterface::Ptr create(const std::string& address, unsigned short port, int con_fd);

  explicit IpUptaneSecondary(const std::string& address, unsigned short port, EcuSerial serial,
                             HardwareIdentifier hw_id, PublicKey pub_key);

  // It looks more natural to return const EcuSerial& and const Uptane::HardwareIdentifier&
  // and they should be 'const' methods


They are const methods now so that comment can be updated.

pattivacek

There are a lot of TODOs here, and historically we only allowed those if they were associated with a ticket to actually investigate them. (That's one of the things the LGTM check broadly helps with.) You don't need to do that before we merge this, but it would be good to follow up on those.

Most of my comments here are small things, so hopefully it won't take long to resolve them. Generally looks pretty good, and we've both tested it, so I know it's close.

pattivacek · 2020-01-13T13:59:18Z

src/libaktualizr/package_manager/packagemanagerconfig.h

+  // optional, if not specified nothing is updated, just image data are verified and delivered
+  boost::filesystem::path filepath;
+  // optional, if not specified any target can be applied
+  std::string target_name;


Just to confirm our conversation: I'd like to try to keep these out for now. target_name can just be the device ID, and filepath can basically be hardcoded for now.

pattivacek · 2020-01-13T14:00:55Z

src/libaktualizr/primary/sotauptaneclient.cc

  if (!storage->loadEcuReportCounter(&ecu_cnt) || (ecu_cnt.size() == 0)) {
    LOG_ERROR << "No ECU version report counter, please check the database!";
+    // should we send manifest at all in this case ?


This might just have to be a TODO for now.

pattivacek · 2020-01-13T14:01:26Z

src/libaktualizr/primary/sotauptaneclient.cc

    } else {
-      LOG_ERROR << "Secondary manifest is corrupted or not signed, manifest: " << secmanifest;
+      // should we report to BE about it ?


This should be a TODO linked to that ticket in some way.

pattivacek · 2020-01-13T14:05:51Z

src/libaktualizr/primary/sotauptaneclient.cc

+      // download an ostree revision just for Primary, Secondary will do it by itself
+      // Primary cannot verify downloaded OSTree targets for Secondaries,
+      // Downloading of Secondary's ostree repo revision to the Primary's can fail
+      // if they differ signficantly as ostree has a certain cap/limit of the diff it pulls


Can you explain this bit about "ostree has a certain cap/limit of the diff it pulls"? I wasn't aware of that.

To me, the comment about "Downloading of Secondary's ostree repo revision to the Primary" is a bit confusing, because the larger conceptual problem is that the Primary shouldn't be downloading objects for the Secondary if it has no use for them and they would just waste space.

the Primary shouldn't be downloading objects for the Secondary if it has no use for them and they would just waste space.

That's exactly what we need this if statement if (update.IsForEcu(primary_ecu_serial) || !update.IsOstree()) for, i.e. to avoid downloading of an ostree revision aimed for a secondary's repo to a primary's ostree repo. The purpose of this comment is to justify this of statement.

Fair enough, I was just a bit thrown off. But is it really true that ostree limits the size of a diff it pulls?

Can you explain this bit about "ostree has a certain cap/limit of the diff it pulls"? I wasn't aware of that.

`Dec 12 17:27:56 qemux86-64 aktualizr while pulling image: 0 Writing content object: min-free-space-percent '3%' would be exceeded, at least 12.2 MB requested

Error message while pulling secondary's ostree repo version/revision/commit-objects to the primary's one if there is big diff between two of them`

https://github.com/ostreedev/ostree/blob/1645416aac802d9dde204117d8fd6352d3958f4a/src/libostree/ostree-repo-commit.c#L442

I ran into it a few times on qemu when was testing an ostree update on secondary at the initial stage of development of the ostree update on secondary.

Gotcha. That error is just because the primary is running out of space, though; that isn't specifically dependent on the size of the diff. I've seen something like "error 34 need more input" which I was hoping was related, but I still don't always really understand why I see that.

That error is just because the primary is running out of space, though; that isn't specifically dependent on the size of the diff.

My understanding is that the error depends on both an available free space on a storage and a diff size (https://salsa.debian.org/debian/ostree/commit/1f5ce1a9f789d9c0de5d6fbdf79540bf71c5bc9b, "specifies a minimum
percentage of total space (in blocks) in the underlying filesystem to
keep free. The default value is 3 ")

also, I get an error when I am trying to pull an ostree revision of the qemu secondary/primary to a local ostree generated by the makephysical tool.

src/libaktualizr/primary/sotauptaneclient.cc

pattivacek · 2020-01-13T14:12:20Z

src/libaktualizr/utilities/utils.cc

@@ -275,7 +275,8 @@ std::string Utils::genPrettyName() {
 std::string Utils::readFile(const boost::filesystem::path &filename, const bool trim) {
  boost::filesystem::path tmpFilename = filename;
  tmpFilename += ".new";
-
+  // that's kind of dangerous to include a specific use-case business logic into a generic function used by many use
+  // cases


What would you prefer instead? If you see an actual problem, it should probably have a TODO associated with it.

Just read a file in Utils::readFile and introduce another function that removes the '.new' file and utilizes the "clean" version of Utils::readFile. Probability that actual problem occurs is very low, but the checking and removing of the ".new" file is redundant/useless in most of the places this function is called from.

- Each target listed in Director's target file should have corresponding match in ImageRepo's one - Test for the use-case if IP Secondary Director's target file and ImageRepo's one have different number of targets - Separate the ostree specific tests from non-ostree one in the IP Secondary Verification test suite Signed-off-by: Mike Sul <[email protected]>

…r IP Secondary Signed-off-by: Mike Sul <[email protected]>

lgtm-com · 2020-01-13T16:01:00Z

This pull request fixes 1 alert when merging b90c955 into 4c41de8 - view on LGTM.com

fixed alerts:

1 for FIXME comment

pattivacek

Looks good once CI passes!

mike-sul · 2020-01-13T16:53:23Z

Added corresponding record into CHANGELOG.md

lgtm-com · 2020-01-13T17:27:45Z

This pull request fixes 1 alert when merging 3bcf5c5 into 4c41de8 - view on LGTM.com

fixed alerts:

1 for FIXME comment

eu-siemann · 2020-01-13T20:10:25Z

src/libaktualizr/bootloader/bootloader.h

@@ -27,7 +31,7 @@ class Bootloader {
 private:
  const BootloaderConfig config_;

-  INvStorage& storage_;
+  std::shared_ptr<INvStorage> storage_;


What was the reason for this change? shared_ptr makes it much harder to understand the lifetime of an object.

What was the reason for this change?

make it consistent with the way the other components "owns" a pointer to the storage instance

it imposes an additional burden to the bootloader clients to take care about the storage instance lifetime

eu-siemann · 2020-01-13T20:16:00Z

src/libaktualizr/campaign/campaign.h

+  static std::vector<Campaign> campaignsFromJson(const Json::Value &json);
+  static void JsonFromCampaigns(const std::vector<Campaign> &in, Json::Value &out);
+  static std::vector<Campaign> fetchAvailableCampaigns(HttpInterface &http_client, const std::string &tls_server);
+


While it seems logical to make those static methods, do we need a namespace for a single class now?

I made them static members to make make docs (doxygen) happy, for some reason it cannot see these three functions implementation if it is just in a scope of the namespace.

do we need a namespace for a single class now?

I think, it can be removed now.

eu-siemann · 2020-01-13T20:25:20Z

Overall, it's very hard to review and to follow the changes. Things, like renaming a class or changing a class member, should be in separate commits with a clear description. I'm fine with merging new functionality as a single commit if it passes basic tests, but changes to the main codebase should be separated, so we can easily track them and revert if needed.

mike-sul · 2020-01-14T08:10:43Z

Overall, it's very hard to review and to follow the changes. Things, like renaming a class or changing a class member, should be in separate commits with a clear description. I'm fine with merging new functionality as a single commit if it passes basic tests, but changes to the main codebase should be separated, so we can easily track them and revert if needed.

fair enough, will try not to produce such huge PRs going forward and break changes into smaller commits.

eu-siemann · 2020-01-14T12:42:44Z

I briefly reviewed all the changes and apart from commit history my main question is about introducing shared pointers for bootloader and storage. I still don't understand the reason for that and, in general, I'd really like to avoid them. IMO, what we want to see in the future is a clear class hierarchy with transparent object ownership and lifetime model and not a situation when everybody has shared pointers to each other.

- Decouple the secondary interface implementation (aktualizr_secondary) from the TCP server - Decouple the secondary implementation common functionality from an update/install method specific (update agent) - Encapsulate the manifest management functionality in a single component and reuse it for Primary and Secondary - Remove some outdated tests for the secondary and add more relevant tests Signed-off-by: Mike Sul <[email protected]>

lgtm-com · 2020-01-14T15:43:39Z

This pull request fixes 1 alert when merging 0a74dda into 21c8595 - view on LGTM.com

fixed alerts:

1 for FIXME comment

pattivacek · 2020-01-17T12:54:22Z

Replaced by #1518.

mike-sul added the not-ready label Dec 23, 2019

mike-sul force-pushed the feat/OTA-4174/direct-ostree-update-ip-secondary branch from 8828cf4 to b404658 Compare December 31, 2019 06:44

mike-sul force-pushed the feat/OTA-4174/direct-ostree-update-ip-secondary branch from b404658 to 6fbbcc0 Compare December 31, 2019 15:12

mike-sul force-pushed the feat/OTA-4174/direct-ostree-update-ip-secondary branch from 6fbbcc0 to 78752fd Compare December 31, 2019 16:04

mike-sul force-pushed the feat/OTA-4174/direct-ostree-update-ip-secondary branch from 78752fd to 4548419 Compare December 31, 2019 21:20

mike-sul removed the not-ready label Jan 2, 2020

pattivacek reviewed Jan 2, 2020

View reviewed changes

mike-sul added the not-ready label Jan 2, 2020

mike-sul force-pushed the feat/OTA-4174/direct-ostree-update-ip-secondary branch from 4548419 to d5af374 Compare January 2, 2020 20:16

mike-sul force-pushed the feat/OTA-4174/direct-ostree-update-ip-secondary branch 2 times, most recently from 4cb1ac7 to 1a9be9a Compare January 3, 2020 10:30

mike-sul force-pushed the feat/OTA-4174/direct-ostree-update-ip-secondary branch from 1a9be9a to 1446fbb Compare January 3, 2020 11:25

mike-sul removed the not-ready label Jan 3, 2020

mike-sul requested a review from lbonn January 3, 2020 15:16

pattivacek mentioned this pull request Jan 6, 2020

OTA-4174: Make an ostree update a default on IP Secondary advancedtelematic/meta-updater#656

Merged

pattivacek requested review from eu-siemann, kbushgit and xcheng-here January 6, 2020 08:52

pattivacek mentioned this pull request Jan 9, 2020

OTA-4174: IP-Secondary: match Director's and Imagerepo's targets #1513

Closed

mike-sul force-pushed the feat/OTA-4174/direct-ostree-update-ip-secondary branch from 3df3e99 to 0f8a6c1 Compare January 13, 2020 10:38

mike-sul force-pushed the feat/OTA-4174/direct-ostree-update-ip-secondary branch from 0f8a6c1 to 1a34721 Compare January 13, 2020 11:43

pattivacek reviewed Jan 13, 2020

View reviewed changes

Mike Sul added 2 commits January 13, 2020 17:18

OTA-4174: Initial dirty version of an ostree direct update support fo…

4477a3a

…r IP Secondary Signed-off-by: Mike Sul <[email protected]>

mike-sul force-pushed the feat/OTA-4174/direct-ostree-update-ip-secondary branch from 1a34721 to b90c955 Compare January 13, 2020 15:23

pattivacek previously approved these changes Jan 13, 2020

View reviewed changes

mike-sul dismissed pattivacek’s stale review via 3bcf5c5 January 13, 2020 16:51

mike-sul force-pushed the feat/OTA-4174/direct-ostree-update-ip-secondary branch from b90c955 to 3bcf5c5 Compare January 13, 2020 16:51

pattivacek previously approved these changes Jan 13, 2020

View reviewed changes

eu-siemann reviewed Jan 13, 2020

View reviewed changes

pattivacek mentioned this pull request Jan 14, 2020

Reduce repetitive logging to debug level. #1516

Merged

mike-sul dismissed pattivacek’s stale review via 0a74dda January 14, 2020 14:59

mike-sul force-pushed the feat/OTA-4174/direct-ostree-update-ip-secondary branch from 3bcf5c5 to 0a74dda Compare January 14, 2020 14:59

pattivacek mentioned this pull request Jan 15, 2020

Changelog: update with recent PRs. #1517

Merged

pattivacek closed this Jan 17, 2020

mike-sul deleted the feat/OTA-4174/direct-ostree-update-ip-secondary branch March 18, 2020 08:55


		#include "package_manager/ostreemanager.h"

		// TBD: consider moving this and SotaUptaneClient::secondaryTreehubCredentials() to encapsulate them in one place that

feat/ota-4174/direct ostree update of IP Secondary #1500

feat/ota-4174/direct ostree update of IP Secondary #1500

Conversation

mike-sul commented Dec 23, 2019 • edited Loading

codecov-io commented Dec 30, 2019 • edited Loading

Codecov Report

lgtm-com bot commented Dec 30, 2019

lgtm-com bot commented Dec 31, 2019

lgtm-com bot commented Dec 31, 2019

lgtm-com bot commented Dec 31, 2019

lgtm-com bot commented Dec 31, 2019

pattivacek left a 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

pattivacek left a 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

lbonn Jan 7, 2020 • edited Loading

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

mike-sul Jan 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Jan 2, 2020

lgtm-com bot commented Jan 3, 2020

lgtm-com bot commented Jan 3, 2020

pattivacek commented Jan 9, 2020

lgtm-com bot commented Jan 13, 2020

lgtm-com bot commented Jan 13, 2020

pattivacek left a 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

pattivacek left a 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

mike-sul Jan 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-sul Jan 13, 2020 • edited Loading

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

lgtm-com bot commented Jan 13, 2020

pattivacek left a comment

Choose a reason for hiding this comment

mike-sul commented Jan 13, 2020

lgtm-com bot commented Jan 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-sul Jan 14, 2020 • edited Loading

Choose a reason for hiding this comment

eu-siemann commented Jan 13, 2020

mike-sul commented Dec 23, 2019 •

edited

Loading

codecov-io commented Dec 30, 2019 •

edited

Loading

lbonn Jan 7, 2020 •

edited

Loading

mike-sul Jan 3, 2020 •

edited

Loading

mike-sul Jan 13, 2020 •

edited

Loading

mike-sul Jan 13, 2020 •

edited

Loading

mike-sul Jan 14, 2020 •

edited

Loading