Adopt a consistent behavior when adding targets and paths #1008

sechkova · 2020-03-26T17:14:07Z

Fixes issue # : #957, #963

Description of the changes being introduced by the pull request:

As described in the issues above, the methods of Targets class use inconsistent checks when adding target files and delegated paths. As a result of this plus the need to avoid access to the file system in certain cases, this pull request suggests:

Adopt the strict behavior of accepting only relative paths (all absolute paths raise an error, even existing ones)
Assume that relative paths are relative to the targets directory and targets directory is not included in the path/target name
Perform the checks only on the path/target name, without accessing the file system
If the added target file does not exist relative to the targets dir, an error is raised at a later stage, during the hash calculation in write() or writeall()
If the path/pattern added to a delegated role does not exist relative to the targets dir, target file won't be matched during a client update (same as current behavior)

Code changes:

Add _check_relpath() method to Targets class
Modify all methods adding paths (add_paths(), delegate()) to utilise _check_relpath()
Modify all methods adding targets (add_target(s)(), delegate(), delegate_hashed_bins(), _locate_and_update_target_in_bin) to utilise _check_relpath()

Note: Tests, delegation section in tutorial as well as get_filepaths_in_directory() helper also need updating but I am sharing the PR sooner as it includes changes needed by PEP 458 implementation. I will be updating them accordingly either in the PR or as a follow on it.

Please verify and check that the pull request fulfills the following
requirements:

The code follows the Code Style Guidelines
Tests have been added for the bug fix or new feature
Docs have been added for the bug fix or new feature

lukpueh

Thanks a ton for this beautiful PR, @sechkova. 💯 for clean and well-documented code and great commit-discipline.

I have left a few very minor inline comments and would like to share and discuss some high-level thoughts about the _check_relpath function. Note that there is no need to fix inline comments before we have agreed on desired behavior.

Let's first agree on what we want here:

Remove file existence checks in add_target* and add_paths methods (write/writeall should deal with that).
Make sure that the paths that end-up in roledb have forward slash path separators and don't start with a leading forward slash. Although the spec only recommends this (see spec#67) python-tuf seems to enforce this (see various path.replace('\\', '/')).

While you already fixed goal 1. (hooray ✅ ), I see a few minor issue in regards to goal 2.

isabs, split(os.path) and normpath all behave differently on Unix and Windows, so the warning/erroring conditions and normalization behavior vary depending on the combination of used platform and passed path.
I wonder if the mixed behavior of warning/erroring/normalizing won't confuse the caller.

Let's back up a little, I think there are two ways to achieve what we want (part 2 above).

expect the caller to pass the right path (/ as separator, no leading /), no matter the platform, and raise an error if they don't:
- Pro: clear behavior (no hidden normalizing magic)
- Con: inconvenient for the caller, e.g. if they want to pass the return value of tuf's
  get_filepaths_in_directory (returns abs paths), or if they are on windows and use (listdir or walk).
take anything and normalize, i.e. convert separators to / and remove leading /.
- Pro: convenient for the caller
- Con: unclear behavior (too much magic)

I strongly lean towards solution 1 (least surprise) plus providing normalizing utility functions for the caller's convenience (can be in a separate PR). What do others think?

Btw. in the future we might be even more sophisticated and have separate checker functions for target path, i.e. path-relative-scheme-less-URL, and path pattern, i.e. path-relative-scheme-less-URL but with glob pattern symbols. (see spec#67).

lukpueh · 2020-03-27T08:31:54Z

tuf/repository_tool.py

+    tuf.formats.RELPATH_SCHEMA.check_match(pathname)
+
+    if os.path.isabs(pathname):
+      raise tuf.exceptions.InvalidNameError(repr(pathname) + ' contains a leading'


Minor nit: Could you please wrap before "leading", so that we don't exceed 80 characters per line. (note to myself: we need to fix the linter configuration!)

Fixed in 9eef0bd

lukpueh · 2020-03-27T08:42:38Z

tuf/repository_tool.py

+      separator.
+
+    <Side Effects>
+      Normalizes pathname.


Minor nit: I wouldn't call this a side effect, as it doesn't change any state outside of this function (as e.g. I/O or modifying some globals would do). I know that other function docstrings in tuf sometimes get this wrong. Please remove.

Fixed in 9eef0bd

lukpueh · 2020-03-27T10:45:25Z

tuf/repository_tool.py

@@ -2772,6 +2755,61 @@ def _locate_and_update_target_in_bin(self, target_filepath, method_name):
          ' in any of the bins.')


+  def _check_relpath(self, pathname):


Minor: If we do normalize paths we should call this function something like _normalize_path.

Fixed in 9eef0bd

lukpueh · 2020-03-27T10:51:51Z

tuf/repository_tool.py

-          ' a valid file in the repository\'s targets'
-          ' directory: ' + repr(self._targets_directory))
+      logger.debug('Replacing target: ' + repr(relative_path))
+      roleinfo['paths'].update({relative_path: custom})


Looks like the if/else is only needed for the debug message and we can put roleinfo['paths'].update({relative_path: custom}) below it (only once).

Already fixed by merging #1007

lukpueh · 2020-03-27T10:52:25Z

tuf/repository_tool.py

+      roleinfo['paths'].update({relative_path: custom})
+
+    tuf.roledb.update_roleinfo(self._rolename, roleinfo,
+      repository_name=self._repository_name)


Minor nit: Please use double indentation (4 spaces) for line continuation (here and elsewhere too).

Already fixed by merging #1007

lukpueh · 2020-03-27T10:54:20Z

tuf/repository_tool.py

+      Normalizes pathname.
+
+    <Returns>
+      The normazlied 'pathname'.


s/normazlied/normalized

Fixed in 9eef0bd

lukpueh · 2020-03-27T10:55:01Z

tuf/repository_tool.py

-        logger.warning(repr(path) + ' is not located in the repository\'s'
-          ' targets directory: ' + repr(self._targets_directory))
+      # Check if the delegated paths or glob patterns are relative and
+      # normalize them. Paths' existense on the file system is not verified.


s/existense/existence

Fixed in 9eef0bd

lukpueh · 2020-03-30T11:07:43Z

I'd love to hear what others think about the two approaches I suggested above...

expect the caller to pass the right path (/ as separator, no leading /), no matter the platform, and raise an error if they don't.

take anything and normalize, i.e. convert separators to / and remove leading /.

See #1008 (review) for more details.

(ping @mnm678, @SantiagoTorres, @trishankatdatadog)

trishankatdatadog · 2020-03-30T20:30:10Z

I'd love to hear what others think about the two approaches I suggested above...

Approach #2 seems friendlier, although some debug/info/warning notices to the logger might help users debug unexpected issues.

mnm678 · 2020-03-30T21:01:45Z

I think #1 would be easier to implement on a wide range of systems. It adds a bit of work for the caller, but eliminates confusing errors when the path doesn't match a known format.

lukpueh · 2020-04-01T09:50:29Z

Thanks for your 4 cents, @mnm678 and @trishankatdatadog. I think Marina and I outvote you in the favor of a strict approach, Trishank. :P I really feel like we should have a function that accepts exactly and only what the specification recommends.

Nonetheless, I do share the friendliness concern, which I think we can accommodate by providing normalization tooling, such as get_target_paths_in_directory(path_to_dir), or normalize_file_paths(list_of_file_paths).

What's your take on this @sechkova and @joshuagl?

joshuagl · 2020-04-01T10:18:08Z

I'm in favour of the strict approach with supporting helper functions; so that we follow the principle of least surprise, match the specification and support client developers (with the helper functions).

sechkova · 2020-04-01T10:50:32Z

Thanks a lot everyone for the helpful comments and suggestions!
I vote in favour of a strict approach + helpers too. Let me try and summarize to help moving the work forward.

Related to path verification:

expect the caller to pass the right path (/ as separator, no leading /), no matter the platform, and raise an error if the path (seen as a string) does not match
do all file system access operations on the path (if any) on write(), writeall() and raise an error in case of non-existing files etc ✅.

In terms of adding normalization functions, how do you feel about:

adding normalize_file_paths(list_of_file_paths) which would work only on paths as strings (no file system access)
keeping get_target_paths_in_directory(path_to_dir) which already performs checks on the file systems in case the caller needs such functionality + maybe adding a call to the above normalize_file_paths

@lukpueh Parts of the tutorial seem to be suggesting absolute paths, which will be rejected by the new approach. Do you think the normalization tooling + updating the tutorials about its usage is appropriate for a separate PR?

lukpueh · 2020-04-01T12:01:13Z

expect the caller to pass the right path (/ as separator, no leading /), no matter the platform, and raise an error if the path (seen as a string) does not match

Yes, I think this is a good start.

(Later in a follow-up PR, we should consider to only accept path-relative-scheme-less-URL strings for target paths (see TARGETPATH in spec), and something similar but with shell-style wildcards allowed for path patterns (see PATHPATTERN in spec)).

do all file system access operations on the path (if any) on write(), writeall() and raise an error in case of non-existing files etc ✅.

Yes!

adding normalize_file_paths(list_of_file_paths) which would work only on paths as strings (no file system access)

Yes! But we can do this in a follow-up PR.

keeping get_target_paths_in_directory(path_to_dir) which already performs checks on the file systems in case the caller needs such functionality + maybe adding a call to the above normalize_file_paths

get_target_paths_in_directory doesn't exist yet. You probably mean get_filepaths_in_directory? I think we can leave that. And maybe add something like:

 def get_normalized_target_paths_in_directory(path):
    return normalize_file_paths(get_filepaths_in_directory(path)):

We can also do that in in a follow-up PR.

@lukpueh Parts of the tutorial seem to be suggesting absolute paths, which will be rejected by the new approach. Do you think the normalization tooling + updating the tutorials about its usage is appropriate for a separate PR?

Hm. I'd rather keep the tutorial usable and not fail the tests. Maybe we can add a quick intermediate fix that replaces the get_filepaths_in_directory with a hard-coded list of the paths it would return but in the format expected by the new add targets functions. What do you think?

sechkova · 2020-04-01T12:26:50Z

get_target_paths_in_directory doesn't exist yet. You probably mean get_filepaths_in_directory?

Yes 👀

@lukpueh Parts of the tutorial seem to be suggesting absolute paths, which will be rejected by the new approach. Do you think the normalization tooling + updating the tutorials about its usage is appropriate for a separate PR?

Hm. I'd rather keep the tutorial usable and not fail the tests. Maybe we can add a quick intermediate fix that replaces the get_filepaths_in_directory with a hard-coded list of the paths it would return but in the format expected by the new add targets functions. What do you think?

Agree on keeping the tutorials consistent with the code and the tests working. A hard-coded list will do the job for now. I'll add it in the PR 👍

joshuagl

Thanks for the PR @sechkova, great work.

I've made a few minor inline comments, mostly just clarifying code comments in the tests.

joshuagl · 2020-04-03T15:31:32Z

tests/test_repository_tool.py

+    # # Test invalid filepath argument (i.e., non-existent or invalid file.)
+    # self.assertRaises(securesystemslib.exceptions.Error,
+    #                   self.targets_object.remove_target,
+    #                   '/non-existent.txt')


There's no need to leave this commented out code, could you remove it?

Fixed in squashed commit 3a13b73

joshuagl · 2020-04-03T15:32:42Z

tests/test_repository_tool.py

+    # Test for delegated targets that do not exist.
+    # An exception should not be raised for non-existent delegated paths,
+    # since at this point the file system should not be accessed yet
+    self.targets_object.delegate(rolename, public_keys, [], threshold,
+        terminating=False, list_of_targets=['non-existent.txt'],
+        path_hash_prefixes=path_hash_prefixes)


I'm not sure this test still makes sense?

I kept the 'non-existent' target checks in all related tests just to make sure that the file system is not accessed at this point and no exception is raised.

joshuagl · 2020-04-03T15:33:36Z

tests/test_repository_tool.py

+        self.targets_object.delegate, rolename, public_keys, [],
+        list_of_targets=['/file1.txt'])
+
+    # A path or target starting with a directory separator


This comment doesn't seem right, aren't we testing Windows path separators here?

Fixed in squashed commit 3a13b73

joshuagl · 2020-04-03T15:35:03Z

tests/test_repository_tool.py

+    self.assertRaises(securesystemslib.exceptions.FormatError,
+        self.targets_object._check_path, 3)
+
+    # Test improperly formatted rolename argument.


Same here, is this comment correct?

Fixed in squashed commit 3a13b73

joshuagl · 2020-04-03T15:36:09Z

tests/test_repository_tool.py

+    self.targets_object._check_path('non-existent.txt')
+    self.targets_object._check_path('subdir/non-existent')
+
+    # Test improperly formatted rolename argument.


This comment doesn't seem to match the code, does it?

Fixed in squashed commit 3a13b73

joshuagl · 2020-04-03T15:38:09Z

tests/test_tutorial.py

-        os.path.abspath(os.path.join('repository', 'targets', 'file1.txt')),
-        os.path.abspath(os.path.join('repository', 'targets', 'file2.txt')),
-        os.path.abspath(os.path.join('repository', 'targets', 'file3.txt'))])
+    # List of targets is hardcoded since get_filepaths_in_directory()


Is this a TODO item to switch to using get_filepaths_in_directory + a helper at some point? The comment doesn't make a lot of sense once the diff showing the deleted call to get_filepaths_in_directory isn't visible. What do you think?

Agree, I tried to improve the comment and keep the TODO idem: 6d8a84d

joshuagl · 2020-04-03T15:41:28Z

docs/TUTORIAL.md

 >>> target4_filepath = os.path.abspath("repository/targets/myproject/file4.txt")
 >>> octal_file_permissions = oct(os.stat(target4_filepath).st_mode)[4:]
 >>> custom_file_permissions = {'file_permissions': octal_file_permissions}
->>> repository.targets.add_target(target4_filepath, custom_file_permissions)
+>>> repository.targets.add_target('myproject/file4.txt', custom_file_permissions)


Is it just me or does this pattern of assigning the full path to a variable and then passing the relative sub-path as a string look a little strange?

Agree, it looks awkward but it is also a good example why a normalising path helper is needed :)
I tired to rewrite it in a more reasonable way: 6d8a84d

joshuagl · 2020-04-03T15:41:57Z

tests/test_tutorial.py

@@ -243,7 +240,7 @@ def test_tutorial(self):
        'repository', 'targets', 'myproject', 'file4.txt'))
    octal_file_permissions = oct(os.stat(target4_filepath).st_mode)[4:]
    custom_file_permissions = {'file_permissions': octal_file_permissions}
-    repository.targets.add_target(target4_filepath, custom_file_permissions)
+    repository.targets.add_target('myproject/file4.txt', custom_file_permissions)


Same as (several comments) above, does this pattern look a little strange?

Same as comment above: 6d8a84d

joshuagl · 2020-04-03T15:45:49Z

tuf/repository_tool.py

@@ -2526,11 +2523,15 @@ def delegate_hashed_bins(self, list_of_targets, keys_of_hashed_bins,
      ordered_roles.append(role)

    for target_path in list_of_targets:
+      # Check if the target path is relative and normalize it. File's existence


_check_path() no longer does any normalising, right? Could you update the comment here?

Fixed during merge in ba8ffd2

sechkova · 2020-04-07T12:49:49Z

Thank you @joshuagl for actually reading all comments sections!
Fixes for your comments and for the tests failing on Windows are added.

lukpueh

Many many thanks for the updates, @sechkova! I have left three minor comments. Please address and we are good to go here. :)

lukpueh · 2020-04-08T10:45:24Z

docs/TUTORIAL.md

 >>> custom_file_permissions = {'file_permissions': octal_file_permissions}
->>> repository.targets.add_target(target4_filepath, custom_file_permissions)
+>>> repository.targets.add_target('target4_filepath', custom_file_permissions)


This should be the variable target4_filepath and not the string, akin to test_tutorial.py

If we only had a tutorial test that actually tests the tutorial document (see #775 (review) and #808 (comment)) ... :)

Thanks for noticing, fix is squashed in 87e1e11

lukpueh · 2020-04-08T11:21:12Z

tuf/repository_tool.py

+      raise tuf.exceptions.InvalidNameError('Path ' + repr(pathname)
+          + ' does not use the forward slash (/) as directory separator.')
+
+    if pathname.startswith(os.sep):


I'm pretty sure that this would allow a windows user to pass an absolute path that starts with "/", which we also don't want. I suggest to change to if pathname.startswith('/'):. Or am I missing something?

See comment below

lukpueh · 2020-04-08T11:23:06Z

tests/test_repository_tool.py

+    # Test invalid pathname - starting with os separator
+    pathname = os.sep + 'file1.txt'
+    self.assertRaises(tuf.exceptions.InvalidNameError,
+        self.targets_object._check_path, pathname)


This test will raise for different reasons on *nix and windows, as a consequence we don't trigger the starts with "/" condition when testing on windows. (also see comment in _check_path)

Agree, the commit where I added os.sep in the tests is reverted and instead _check_path uses if pathname.startswith('/') as suggested, thanks!
Also added an additional test case for paths starting with '\', just in case for the future.
The new commit: e85a3b3

sechkova · 2020-04-08T13:33:04Z

Rebased and new changes added

lukpueh

Just stumbled across two more things that probably came from a rebase on some other work (plus a a minor docstring request). Would you mind fixing those? :)

lukpueh · 2020-04-08T14:51:57Z

tuf/repository_tool.py

@@ -1854,6 +1854,8 @@ def add_paths(self, paths, child_rolename):
      securesystemslib.exceptions.Error, if 'child_rolename' has not been
      delegated yet.

+      tuf.exceptions.InvalidNameError, if 'pathname' does not match pattern.


Minor nit: Would you mind replacing 'pathname' with what's actually being checked, that is here "any of the passed 'paths'". (same comment applies to other occurrences of this line).

lukpueh · 2020-04-08T14:55:15Z

tuf/repository_tool.py

+      # forward slash as a separator or raise an exception. Paths' existence
+      # on the file system is not verified. If the path is incorrect,
+      # the targetfile won't be matched successfully during a client update.
+      self._check_path(path)


It seems like we don't need the relative_paths variable anymore.

lukpueh · 2020-04-08T14:59:20Z

tuf/repository_tool.py

-          filepath[targets_directory_length + 1:].replace('\\', '/'))
-
+      self._check_path(target)
+      relative_list_of_targets.append(target)


Same as above in add_paths. Given that we don't do no normalizing anymore, we don't have to copy the target paths to a new list, i.e. relative_list_of_targets. Please remove.

This method checks if the passed path is relative and returns the normalized path. Checks are performed without accessing the file system. Signed-off-by: Teodora Sechkova <[email protected]>

Adopt the same behavior in all methods of Targets class (add_paths(), delegate()) that add new paths to a role. Signed-off-by: Teodora Sechkova <[email protected]>

Adopt the same path verification strategy in all methods of Targets class that add new target files by utilizing the _check_relpath() method. Signed-off-by: Teodora Sechkova <[email protected]>

Adopt a stict behavor allowing only paths that match the definition of a PATHPATTERN or aTARGETPATH (use the forward slash (/) as directory separator and do not start with a directory separator). Raise an exception otherwise. Rename the method to a more general _check_path(). Signed-off-by: Teodora Sechkova <[email protected]>

Normalization of the target path is no longer necessary during metadata generation, since an exception is raised earlier, on the addition of an incorrect target path or pattern. Signed-off-by: Teodora Sechkova <[email protected]>

Replace the absolute paths returned by get_filepaths_in_directory() in the tutorial with a hard-coded list of relaive filepaths since add_target(s) and delegate() methods raise excception on absolute paths. Remove an obsolete warning about path pattern's location. Signed-off-by: Teodora Sechkova <[email protected]>

- add a test for _check_path() method of Targets class. - update all tests calling _check_path() respectively - update test_tutorial Signed-off-by: Teodora Sechkova <[email protected]>

Test is updated to include checks for incorrect target paths. Signed-off-by: Teodora Sechkova <[email protected]>

Improve the coding style in TUTORIAL in the case where absolute path to a file is needed to perform file system access and at the same time is rejected by Targets methods. Signed-off-by: Teodora Sechkova <[email protected]>

Use a hard-coded unix separator ('/') so that an exception is also raised for paths starting with '/' when executing on Windows systems. Update test_check_path to explicitly test invalid paths starting with Windows style separator. Signed-off-by: Teodora Sechkova <[email protected]>

Remove unnecessary copying of paths to another list in add_targets() and add_paths() methods. Fix incorrect docstring text. Signed-off-by: Teodora Sechkova <[email protected]>

sechkova · 2020-04-08T16:03:19Z

Rebased and fixed in 882df8e :)

lukpueh · 2020-04-08T16:48:32Z

Hooray! 🎉 This does look good to me now. Thanks for your patience, @sechkova! I created tickets for the follow-up PRs we discussed above (#1018, #1019). Merging...

lukpueh self-assigned this Mar 27, 2020

lukpueh requested changes Mar 27, 2020

View reviewed changes

lukpueh mentioned this pull request Mar 27, 2020

Enhancements for hashed bin delegation #1007

Merged

3 tasks

lukpueh mentioned this pull request Mar 30, 2020

Support abstract files and directories #1009

Closed

9 tasks

sechkova force-pushed the rework_path_checks branch from 75ee328 to 7617e09 Compare April 3, 2020 13:56

joshuagl reviewed Apr 3, 2020

View reviewed changes

sechkova force-pushed the rework_path_checks branch from 7617e09 to 6d8a84d Compare April 7, 2020 12:35

lukpueh requested changes Apr 8, 2020

View reviewed changes

sechkova force-pushed the rework_path_checks branch from 6d8a84d to e85a3b3 Compare April 8, 2020 13:22

lukpueh reviewed Apr 8, 2020

View reviewed changes

sechkova added 10 commits April 8, 2020 18:58

Add _check_relpath() method to Targets class

2e84c3a

This method checks if the passed path is relative and returns the normalized path. Checks are performed without accessing the file system. Signed-off-by: Teodora Sechkova <[email protected]>

Make methods adding 'paths' consistent

187be96

Adopt the same behavior in all methods of Targets class (add_paths(), delegate()) that add new paths to a role. Signed-off-by: Teodora Sechkova <[email protected]>

Make methods adding targets consistent

368be61

Adopt the same path verification strategy in all methods of Targets class that add new target files by utilizing the _check_relpath() method. Signed-off-by: Teodora Sechkova <[email protected]>

Remove secondary target path normalization

3572e73

Normalization of the target path is no longer necessary during metadata generation, since an exception is raised earlier, on the addition of an incorrect target path or pattern. Signed-off-by: Teodora Sechkova <[email protected]>

Add and update tests for _check_path() method

77f5f73

- add a test for _check_path() method of Targets class. - update all tests calling _check_path() respectively - update test_tutorial Signed-off-by: Teodora Sechkova <[email protected]>

Update test_delegate_hashed_bins()

57b4051

Test is updated to include checks for incorrect target paths. Signed-off-by: Teodora Sechkova <[email protected]>

Update TUTORIAL and test_tutorial

a71b3c2

Improve the coding style in TUTORIAL in the case where absolute path to a file is needed to perform file system access and at the same time is rejected by Targets methods. Signed-off-by: Teodora Sechkova <[email protected]>

Remove unnecessary variables

882df8e

Remove unnecessary copying of paths to another list in add_targets() and add_paths() methods. Fix incorrect docstring text. Signed-off-by: Teodora Sechkova <[email protected]>

sechkova force-pushed the rework_path_checks branch from e85a3b3 to 882df8e Compare April 8, 2020 15:59

lukpueh mentioned this pull request Apr 8, 2020

Add stricter TARGETPATH and PATHPATTERN checks #1018

Closed

lukpueh mentioned this pull request Apr 8, 2020

Provide TARGETPATH normalizing convenience interface #1019

Closed

lukpueh approved these changes Apr 8, 2020

View reviewed changes

lukpueh merged commit 60a4da0 into theupdateframework:develop Apr 8, 2020

This was referenced Apr 8, 2020

Targets' add_target(s) methods do not correctly check passed paths #957

Closed

Conflicting file path assertions in delegate() and add_paths() #963

Closed

sechkova deleted the rework_path_checks branch April 22, 2020 15:21

joshuagl mentioned this pull request May 19, 2020

Misleading / missing steps in TUTORIAL for delegated roles #838

Closed

		@@ -2772,6 +2755,61 @@ def _locate_and_update_target_in_bin(self, target_filepath, method_name):
		' in any of the bins.')


		def _check_relpath(self, pathname):

Adopt a consistent behavior when adding targets and paths #1008

Adopt a consistent behavior when adding targets and paths #1008

Conversation

sechkova commented Mar 26, 2020

lukpueh left a comment • 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

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

lukpueh commented Mar 30, 2020

trishankatdatadog commented Mar 30, 2020

mnm678 commented Mar 30, 2020

lukpueh commented Apr 1, 2020

joshuagl commented Apr 1, 2020

sechkova commented Apr 1, 2020 • edited Loading

lukpueh commented Apr 1, 2020

sechkova commented Apr 1, 2020

joshuagl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sechkova Apr 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sechkova commented Apr 7, 2020

lukpueh 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

sechkova commented Apr 8, 2020

lukpueh 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

sechkova commented Apr 8, 2020

lukpueh commented Apr 8, 2020

lukpueh left a comment •

edited

Loading

sechkova commented Apr 1, 2020 •

edited

Loading

sechkova Apr 7, 2020 •

edited

Loading