First stable release almost ready #68

dbarrosop · 2018-01-06T19:07:00Z

Ok, I understand this is a lot but I am hoping after this the "core" should be stable and new things should just be small new features or bugfixes. I'd recommend reviewing this PR commit by commit or just by checking the tutorial (everything I wrote here with the exception of a couple of new tasks should be covered in the tutorials section):

http://brigade.readthedocs.io/en/rework_examples/tutorials/intro/index.html

I still want to finish the examples but wanted to get the tutorials out first.

@ktbyers @ogenstad

For instance: result = d.run(networking.napalm_validate, raise_on_error=False, src=THIS_DIR + "/data/validate_error.yaml")

…the same host

…ework_examples

ktbyers · 2018-01-06T19:58:31Z

I will try to review in the next few days.

ogenstad · 2018-01-08T20:42:01Z

Same for me, will review soon.

dbarrosop · 2018-01-08T21:08:14Z

thanks to both of you, focus on the tutorial, I think that's more important than the code itself. Will try to update the examples in the /examples folder in the next couple of days as I think that will help as well.

ktbyers

Initial review...only part way through.

ktbyers · 2018-01-10T04:24:53Z

brigade/core/__init__.py

@@ -72,6 +72,7 @@ def __init__(self, inventory, dry_run, config=None, config_file=None,
        logging.basicConfig(
            level=logging.ERROR,
            format=format,
+            filename="brigade.log",


IMO, we shouldn't enable this by default. People can enable it if they want to, but shouldn't default to enabled.

I recognize this changes in this commit 027a440, but still defaults to logging to brigade.log (level INFO).

ktbyers · 2018-01-10T04:36:53Z

examples/hosts.yaml

+    role: spine
+    group: cmh
+    brigade_nos: junos
+    type: network_device


I assume all fields not-named "brigade_xxxxxx" are just arbitrary tags and that any other tags can be valued and used in the Brigade filter.

yeah, anything can be used to filter, both brigade_xxxx and whatever_other_field_i_want. And yes, you can basically have any user defined field you want, everything that doesn't start with brigade_ is user defined and doesn't have special meaning to brigade. With the exception of group which leads me to the following question, should we rename it to brigade_group?

ktbyers · 2018-01-10T04:42:54Z

examples/groups.yaml

@@ -3,20 +3,8 @@ all:
    group: null


This group: null is not intuitive. Also I would expect group: all to just happen automatically.

Does there need to be a clear group hierarchy? For example, bma-leaf belongs to group bma which belongs to group all which belongs to group null.

This group: null is not intuitive

Yeah, not sure why that was there to be honest. Is not needed. It's not anymore.

Also I would expect group: all to just happen automatically.

Yeah

Does there need to be a clear group hierarchy? For example, bma-leaf belongs to group bma which belongs to group all which belongs to group null.

Not at all. Names are irrelevant and completely arbitrary. Those could easily have been "potato", "orange" and "lettuce".

ktbyers · 2018-01-10T04:58:42Z

examples/1_simple_runbooks/backup.py

@@ -0,0 +1,28 @@
+#!/usr/bin/env python
+from brigade.easy import easy_brigade


Why does easy_brigade default dry_run=True? I would default the other way (i.e. it would be a more useful default that way)?

Also I would default the raise_on_error=False (in Config). Given that we are generally running concurrent connections to devices, this is more logical.

mostly defaulting to the safest options which are (1) just give me the changes without changing anything and (2) if something bad happens break! I am happy to discuss alternatives for each option but the reasoning was just simply that "default to the safest scenario if the user ignores these parameters"

I think it is more logical to default to the most common/wanted option, not the safest.

Also there will be plenty of tasks that won't have a dry-run so defaulting to dry-run doesn't make sense as a default.

I think this is the behavior most people would expect also.

re dry_run; sounds fine by me. We can change the default to False. What's your take on raise_on_error?

I agree regarding dry_run. For raise_on_error I don't think it's as important as it was as a host should be marked as skipped for the following tasks it shouldn't be as destructive by default as it could have been previously.

ktbyers · 2018-01-10T05:02:11Z

examples/1_simple_runbooks/backup.py

+                    filename="./backups/{}".format(task.host))
+
+
+brigade = easy_brigade(


Using brigade name here is a confusing name choice since that is the name of the library also.

you are right. Will fix that

ktbyers · 2018-01-10T05:09:18Z

examples/1_simple_runbooks/configure.py

+                 path="../templates/{brigade_nos}")
+    task.host["config"] = r.result
+
+    r = task.run(data.load_yaml,


It would be useful to add one-line comments on what you are doing in some of these steps.

I think we need to be clear on some of this string formatting magic. What happens; how does it work; which variables are supported and why.

yeah, that's explained in the tutorial :P might need a longer explanation though

It's mentioned in the note "Note you can format strings using host data.", but I agree that it would deserve a longer explanation.

ktbyers · 2018-01-10T05:11:34Z

examples/1_simple_runbooks/configure.py

+    task.host["config"] = r.result
+
+    r = task.run(data.load_yaml,
+                 file="../extra_data/{host}/l3.yaml")


{host} variable where does that come from?

ktbyers · 2018-01-10T05:19:04Z

examples/1_simple_runbooks/configure.py

+
+
+brigade = Brigade(
+    inventory=SimpleInventory("../hosts.yaml", "../groups.yaml"),


Isn't hosts.yaml in ../inventory/hosts.yaml?

ktbyers · 2018-01-10T05:21:50Z

examples/1_simple_runbooks/validate.py

@@ -0,0 +1,54 @@
+#!/usr/bin/env python
+"""
+In this example we write a CLI tool with brigade and click to deploy configuration.


Wrong comment.

ktbyers · 2018-01-10T05:29:11Z

examples/2_simple_tooling/configure.py

+from brigade.core import Brigade
+from brigade.core.configuration import Config
+from brigade.plugins.inventory.simple import SimpleInventory
+from brigade.plugins.tasks import data, networking, text


I don't think it makes sense to have four CLI examples. I think one is probably enough.

I wanted to have a few use cases and also I wanted to have the bits necessary to build the more elaborated workflow under 3_elaborated. So the idea of the examples was to show the evolution from a "playbook" to a "simple tool" to a more elaborated "workflow".

I don't think having several examples are bad. It would be nice to be able to test them though.

ktbyers · 2018-01-10T05:47:47Z

@dbarrosop FYI, I just submitted the review up through where I was at. I was just going through the commits one-by-one.

I will see if I can continue reviewing in the next couple of days.

dbarrosop · 2018-01-10T20:25:55Z

@ktbyers sorry about the examples, they are still far from done. I'd recommend starting with the tutorials as they explain the reasoning behind some things.

dbarrosop · 2018-01-10T20:46:27Z

examples/groups.yaml

-
-bma-host:
-    group: bma
-
 bma:
    group: all


Moving this comment here to track it:

@ktbyers said: Also I would expect group: all to just happen automatically

yeah, I agree.

ktbyers · 2018-01-10T20:51:02Z

@dbarrosop No worries, I was just flagging things that I saw and trying to understand things (and why certain choices were being made).

dbarrosop · 2018-01-10T20:53:11Z

brigade/core/__init__.py

        if available_connections is not None:
            self.available_connections = available_connections
        else:
            self.available_connections = connections.available_connections

+    def configure_logging(self):


Moving this message here to track it:

@ktbyers said: IMO, we shouldn't enable this by default. People can enable it if they want to, but shouldn't default to enabled.

ok, let me revisit this.

Yes, that message got hidden a bit because I made it on a commit that was changed in a later commit.

Perhaps is should be controlled through the Config object.

Yeah, i agree with the Config object. I will leave it as it is right now and we can discuss what's the best behavior for this.

ktbyers · 2018-01-11T18:15:29Z

docs/howto/basic-napalm-getters.rst

   :name: hosts.yaml
   :language: yaml

 * Groups file:

-.. literalinclude:: ../../examples/groups.yaml
+.. literalinclude:: ../../examples/inventory/groups.yaml


I am going to add general comments about the tutorial here:

On line 57 it needs to be:

host["brigade_nos"]

The inconsistency between attribute names and keys is a bit strange? Should these be identical?

# dir(host) with underscore attributes filtered out 'brigade', 'connections', 'data', 'get', 'get_connection', 'group', 'host', 'items', 'keys', 'name', 'network_api_port', 'nos', 'os', 'password', 'ssh_port', 'username', 'values' In [9]: host.keys() Out[9]: dict_keys(['name', 'group', 'domain', 'brigade_host', 'brigade_username', 'brigade_password', 'brigade_nos', 'brigade_port', 'site', 'role'])

The part that is a bit strange is that we have names that are similar, but not identical. Should we just make them identical. Also should we have a one-to-one correspondence between attribute access and dictionary keys (i.e. all dictionary keys could just be accessed as host.foo)?

Use of brigade as a variable name here:

>>> brigade = Brigade( ... inventory=SimpleInventory("hosts.yaml", "groups.yaml"), ... dry_run=True)

This doesn't work:

>>> result = cmh_leaf.run(task=tasks.napalm_get_facts, ... facts="facts")

Instead needed to use:

from brigade.plugins.tasks.networking import napalm_get result = cmh_leaf.run(task=napalm_get, getters='get_facts')

print(result) on line 88 doesn't work as shown:

# Here is what I saw at that point: >>> print(result) AggregatedResult: napalm_get

Here is what I ended up doing to print the result:

>>> for k, v in result.items(): ... print("-" * 80) ... print(k) ... for e in v: ... print(e.result) ... print("-" * 80)

I thought it was a bit strange in the output printing above that the result.items() that the v was a MultiResult and I needed to loop over it. The part that was strange to me was when we do the get_task(task) later in this tutorial...in that situation we just get a result that we can print and not a MultiResult (or vice versa). In other words, it isn't clear to me why we return a MultiResult in one context and not in the other.

There were some changes that I needed to make to get the get_info(task) to run properly. Once again for the import of napalm_get and for the result printing:

def get_info(task): # Grouping multiple tasks that go together r = napalm_get(task, "get_facts") print(task.host.name) print("============") print(r.result) r = napalm_get(task, "get_interfaces") print(task.host.name) print("============") print(r.result)

Okay...maybe this is all old as it is in docs/howto and not docs/tutorials. Probably should only have one place and not three (examples, docs/howto, docs/tutorials and I am 0 for 2 on reviewing the right one).

It is due to the structure of the documentation in general. Saw that you were talking about this on Twitter too. Basically howto's aim to solve a specific task and tutorials is more handholding. Currently they might basically cover the same topics. Doesn't look like the howtos are included in this PR, but they probably need to be updated.

Also think that the examples are good for people who only want to view the code and not have it spreadout on a page with different sections as can happen in a tutorial.

ogenstad · 2018-01-12T09:43:12Z

docs/tutorials/intro/inventory.rst

+
+.. note:: For this and the subsequent sections of this tutorial we are going to use the :obj:`SimpleInventory <brigade.plugins.inventory.simple.SimpleInventory>` with the data located in ``/examples/inventory/``. We will also use the ``Vagrantfile`` located there so you should be able to reproduce everything. You can head to `Hosts/Groups contents`_ to see the contents of the file just for reference.
+
+First, let's create the inventory::


Should this say "create", not load?

ogenstad · 2018-01-12T09:50:14Z

docs/tutorials/intro/inventory.rst

+Now let's inspect the hosts and groups we have::
+
+	>>> inventory.hosts
+	{'host1.cmh': Host: host1.cmh, 'host2.cmh': Host: host2.cmh, 'spine00.cmh': Host: spine00.cmh, 'spine01.cmh': Host: spine01.cmh, 'leaf00.cmh': Host: leaf00.cmh, 'leaf01.cmh': Host: leaf01.cmh, 'host1.bma': Host: host1.bma, 'host2.bma': Host: host2.bma, 'spine00.bma': Host: spine00.bma, 'spine01.bma': Host: spine01.bma, 'leaf00.bma': Host: leaf00.bma, 'leaf01.bma': Host: leaf01.bma}


One thing I don't like about this is that the output requires side scrolling to see it all. However I don't know if it would be better to print it vertically either since that will just make the page longer..

Yes, I pretty strongly disliked the horizontal scrolling (it was close to unreadable). It would be much better if it displayed vertically.

ok, will try to fix. I am going to move the tutorial to a jupyter notebook anyway so we can run it and make sure it's still valid. I already moved most of the examples.

ogenstad · 2018-01-12T09:51:17Z

docs/tutorials/intro/inventory.rst

+Not the most useful example but it should be enough to illustrate how it works.
+
+
+Hosts/Groups contents


I think it would be helpful if this was displayed before the inventory was loaded. I think it makes it easier to understand what data we are looking at.

ogenstad · 2018-01-12T09:52:26Z

docs/tutorials/intro/brigade.rst

+
+If you want to use the "raw" API you need two things:
+
+1. A configuration object.


Place a link to the Config object?

ogenstad · 2018-01-12T09:53:30Z

docs/tutorials/intro/running_tasks_errors.rst

+Dealing with task errors
+========================
+
+Tasks can fail due to many reasons. A continuation we will see how to deal with errors effectively with brigade.


The second sentence sounds wrong. "As we continue we will see how.."

ogenstad · 2018-01-12T09:55:23Z

docs/tutorials/intro/running_tasks_errors.rst

+	>>> r.failed
+	True
+	>>> r.failed
+	False


This doesn't look correct?

Yes, I saw that also, is there a missing call to b.run?

215 >>> r = b.run(task_that_sometimes_fails) 216 >>> r.failed 217 True 218 >>> r.failed 219 False

maybe... I will correct as I move it to a jupyter notebook.

ogenstad · 2018-01-12T09:57:16Z

docs/tutorials/intro/running_tasks_errors.rst

+	...           raise_on_error=False)
+	>>>
+
+As you can see, regardless of what ``brigade`` had configured to do, the task failed on the first case but didn't on the second one.


--> of what brigade had been configured to do,

ogenstad · 2018-01-12T09:57:47Z

docs/tutorials/intro/running_tasks_errors.rst

+
+Tasks can fail due to many reasons. A continuation we will see how to deal with errors effectively with brigade.
+
+Failing by default


Stay positive. ;)

ogenstad · 2018-01-12T11:59:07Z

brigade/core/__init__.py

        if available_connections is not None:
            self.available_connections = available_connections
        else:
            self.available_connections = connections.available_connections

+    def configure_logging(self):


Perhaps is should be controlled through the Config object.

ogenstad · 2018-01-12T12:16:56Z

brigade/easy.py

+from brigade.plugins.inventory.simple import SimpleInventory
+
+
+def easy_brigade(host_file="host.yaml", group_file="groups.yaml", dry_run=True, **kwargs):


Would basic_brigade be a better name than easy_brigade? Easy gives the impression that the other one is hard.

Yes, I disliked the easy_brigade name.

Can we just get rid of this whole easy.py and have it be the Brigade class default:

So you can just do:

my_brigade = Brigade()

And then in the class Brigade you would have:

def __init__(self, inventory=None, dry_run=True, config=None, config_file="~/.brigade.yaml", available_connections=None, logger=None, data=None, host_file="hosts.yaml", group_file="groups.yaml"): ... if inventory is None: inventory=SimpleInventory(host_file, group_file),

I think it is more logical also to have a default config_file like ~/.brigade.yaml and not hard-code a configuration option into easy.py.

We could also move the host_file and group_file arguments to be part of Config i.e. default them to "hosts.yaml" and "groups.yaml" in current directory (and then you can overwrite them in Config).

So the idea of easy_brigade (which I am fine renaming, btw) is to provide a simple "constructor" for the most common cases. We can add arguments to accommodate most use cases.

I don't like the idea of accommodating common use cases in the core API because that just adds complexity. For instance, what heppens if you pass an inventory and host_file? You ignore the latter? What if you want to also add log_file and debug? With a quick-to-access function like easy_brigade you have a clear demarcation of what's "business logic" and what's just "core" behavior.

We could also move the host_file and group_file arguments to be part of Config

That's tricky because those apply to a plugin, not to the core itself but I am tempted to agree. We could actually pass the configuration object to the inventory plugin and do something like:

class SimpleInventory(Inventory): def __init__(self, config, host_file=None, group_file=None, **kwargs): """ Configuration: It accepts reading from the configuration the host_file and group_file via simple_inventory_host_file and simple_inventory_group_file """ host_file = host_file or config.simple_inventory_host_file group_file = group_file or getattr(config, "simple_inventory_group_file")

Although if we are going to do that I'd like to integrate better with the Config object so we can also accept env variables (I'd rather use env variables that configuration objects as that works way better with CIs and docker containers). Maybe the Config object should have something like:

Config.get('simple_inventory_host_file', env_name='BRIGADE_SIMPLE_INVENTORY_HOST_FILE', default=None)

In any case, these are two different issues:

Do we want easy_brigade (or fast_brigade, or quick_brigade or whatever other name) to accommodate common use cases with a simple import?

What to move to the configuration object and how to integrate better.

For me it was mostly the name with the easy_ prefix. I think fast_ quick_ basic would be better.

I don't like the idea of accommodating common use cases in the core API because that just adds complexity. For instance, what heppens if you pass an inventory and host_file? You ignore the latter? What if you want to also add log_file and debug? With a quick-to-access function like easy_brigade you have a clear demarcation of what's "business logic" and what's just "core" behavior.

Completely agree with this.

Yeah, I disagree here, but it doesn't matter much.

I am not really seeing the "business-logic" versus "core" distinction i.e. I am not seeing why some things would be categorized one way and others differently.

Two me I think we are creating two entry points into the library and the one we are categorizing as non-core will be the main one. So I think we are unnecessarily adding complexity.

Internally whether we place easy_brigade in the core directory seems irrelevant (since it will be core in practice and will cause end-users code to break if we change it improperly).

So it just seems like we are unnecessarily adding a layer of indirection for assigning default values to the class (and this causes side effects like having to document the two entry points and having to maintain more code).

But it doesn't matter much either way (i.e. it is a small amount of extra code with a pretty simple purpose).

I need to read the second half of your comments again (the comments on groups and hosts and environment variables), but probably won't have time until the weekend or next week.

Ok, I understand where you are coming from and it was probably bad working on my side. What I mean is not that easy_brigade (or whatever we call it) won't be part of the "core", what I meant is that I didn't want to add business logic the Brigade class. I'd rather keep it agnostic and put the business logic somewhere else because that means:

a. we avoid extra complexity were not needed
b. we can provide different entry points for different use cases

As a rule of thumb I like classes and methods with very few arguments, without arguments that conflict with each other (meaning that an argument than when it has a certain value it invalidates some other argument) or with arguments that changes substantially the behavior of the method/class. I'd rather have multiple smaller methods. In summary, if it's hard to test/document, break it down into different methods/classes.

Philosophy aside, you are right pointing out we could probably do the same with default arguments/options so I am going to explore that venue.

ogenstad · 2018-01-12T12:20:42Z

brigade/plugins/tasks/files/write.py

+        return f.read().splitlines()
+
+
+def generate_diff(filename, content, append):


Should read_file and generate_file be prefixed with a _?

If we decide to allow multiple tasks within the same file we might want to collect all of the tasks dynamically, then we could just exclude _read_file and _generate_diff.

you are right, will mark them as private.

ogenstad · 2018-01-12T12:21:24Z

brigade/plugins/tasks/files/write.py

+    return "\n".join(diff)
+
+
+def write(task, filename, content, append=False):


I still think that write sounds very generic. Could it be write_file instead?

I agree. Changing to write_file

ogenstad · 2018-01-12T12:26:39Z

brigade/plugins/tasks/networking/napalm_validate.py

+    """
+    Gather information with napalm and validate it:
+
+        http://napalm.readthedocs.io/en/develop/validate/index.html


This seems misplaced, it looks a bit strange on RTD and we don't have it for the rest of the tasks.

ogenstad · 2018-01-12T12:35:49Z

brigade/plugins/functions/__init__.py

@@ -0,0 +1,6 @@
+from . import text


What other types of function plugins do you see?

Was this some thought about making it easier to run things remotely in the future or wouldn't the print_title function just be a helper within Brigade core?

A function is something that doesn't apply to hosts themselves but are nice to have. I expect git functions as well, for instance. But effectively a function is a helper as you called it. I just wanted to follow same structure as plugins to make clear that they are not part of the core and they provide extra functionality to users, not to tasks or to the core itself.

Ok, sounds good.

dbarrosop · 2018-01-20T16:37:47Z

I think I am going be merging this PR because I think this PR is too big and it's becoming hard to have meaningful discussions around specific topics.

I will be creating issues to explore some of the ideas (like the discussion around easy_brigade) and to finish some of the things I am leaving unfinished. Feel free to keep adding comments here or creating individual github issues for anything that you might want still to discuss. Don't consider this PR done just because it's merged.

dbarrosop · 2018-01-20T16:54:06Z

Ok, I have created a couple of issues and I linked them in the appropriate threads. As mentioned, don't hesitate to keep raising concerns/comments about this PR and feel free to create issues where I may have missed them.

dbarrosop added 25 commits December 21, 2017 13:41

allow passing dry_run to individual tasks

92fed6f

added write task

37e1fa5

redundant

29f4c56

log by default to ./brigade.log

2a926df

raise an error properly if we fail to connect to the device

b0ca3e0

added more devices to the lab

8063186

examples WIP

eadbf11

improvements to napalm_configure

a230151

unnecessary

3184632

improvements to logging

027a440

better error handling

5e59282

document new Result attributes

52f63f9

allow overriding raise_on_error per task

4972edd

For instance: result = d.run(networking.napalm_validate, raise_on_error=False, src=THIS_DIR + "/data/validate_error.yaml")

added napalm_validate task

cecc260

render automatically template path

30e8570

Added helper functions to make output pretty

0b265ce

fix napalm_validate tests

3e89f8c

bugfix

0e4027d

return properly dict_keys and dict_values

ca49110

subtasks now return MultiResults which aggregate mutiple results for …

1e06a18

…the same host

Merge branch 'develop' of github.com:napalm-automation/brigade into r…

4041eec

…ework_examples

skip hosts if they fail and raise_on_error is False

f830286

added easy_brigade

8b8a4d6

added tutorials

a7096c6

Merge branch 'develop' of github.com:napalm-automation/brigade into r…

499d11d

…ework_examples

ktbyers reviewed Jan 10, 2018

View reviewed changes

dbarrosop commented Jan 10, 2018

View reviewed changes

ktbyers reviewed Jan 11, 2018

View reviewed changes

ogenstad reviewed Jan 12, 2018

View reviewed changes

dbarrosop added 6 commits January 15, 2018 21:57

bugfix

308fd67

allow telling a task to be run on hosts that should be skipped otherwise

57853e2

for consistency with tasks

7824871

allow printing multiresult objects

b9a0d09

update docstring

df26d81

updated examples

707c9cc

minor fixes proposed in the comments

7e1bb53

This was referenced Jan 20, 2018

Revisit logging #71

Closed

Alternative to easy_brigade #73

Closed

Rename write task to write_file #74

Closed

adding missing deps

7b9eef3

dbarrosop merged commit 9d190ba into develop Jan 20, 2018

dbarrosop deleted the rework_examples branch January 21, 2018 15:07

		@@ -0,0 +1,28 @@
		#!/usr/bin/env python
		from brigade.easy import easy_brigade

		filename="./backups/{}".format(task.host))


		brigade = easy_brigade(



		brigade = Brigade(
		inventory=SimpleInventory("../hosts.yaml", "../groups.yaml"),


		.. note:: For this and the subsequent sections of this tutorial we are going to use the :obj:`SimpleInventory <brigade.plugins.inventory.simple.SimpleInventory>` with the data located in ``/examples/inventory/``. We will also use the ``Vagrantfile`` located there so you should be able to reproduce everything. You can head to `Hosts/Groups contents`_ to see the contents of the file just for reference.

		First, let's create the inventory::

		Not the most useful example but it should be enough to illustrate how it works.


		Hosts/Groups contents


		If you want to use the "raw" API you need two things:

		1. A configuration object.


		Tasks can fail due to many reasons. A continuation we will see how to deal with errors effectively with brigade.

		Failing by default

		from brigade.plugins.inventory.simple import SimpleInventory


		def easy_brigade(host_file="host.yaml", group_file="groups.yaml", dry_run=True, **kwargs):

		return f.read().splitlines()


		def generate_diff(filename, content, append):

		return "\n".join(diff)


		def write(task, filename, content, append=False):

First stable release almost ready #68

First stable release almost ready #68

Conversation

dbarrosop commented Jan 6, 2018 • edited Loading

ktbyers commented Jan 6, 2018

ogenstad commented Jan 8, 2018

dbarrosop commented Jan 8, 2018

ktbyers 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

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

ktbyers commented Jan 10, 2018

dbarrosop commented Jan 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktbyers commented Jan 10, 2018

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

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

ktbyers Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

dbarrosop Jan 15, 2018 • 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

dbarrosop commented Jan 20, 2018

dbarrosop commented Jan 20, 2018

dbarrosop commented Jan 6, 2018 •

edited

Loading

ktbyers Jan 12, 2018 •

edited

Loading

dbarrosop Jan 15, 2018 •

edited

Loading