bulk REST requests #96

peterthomassen · 2018-03-29T02:27:40Z

Finally!

A few notes:

I tried to make this a few separate commits, to avoid confusion. The main commit may still seem large (but it's a large feature, too). I can try to split it up a bit more if it is too clumsy, but the changes within it are a bit intertwined, so I'm not sure if it is really possible.
There are one or two unrelated commits that I didn't bother to make a separate PR for, as they are not urgent.
One of the e2e tests currently fails due to a minor problem; I'll try to fix it during the next days. I don't think that needs to hold back review.
e2e tests for RRset deletion via bulk requests (setting 'records': []) are missing; I'll add them over the next days.
e2e tests checking DNS output are mostly disabled (status "pending", with a dash) because the DNS library doesn't really give us what we want (not even TTLs, except for A/AAAA). We should try to find another DNS library.

Have fun, and 1000 thanks for reviewing! <3

peterthomassen · 2018-03-29T13:47:55Z

I added e2e tests for RRset deletion via bulk requests (setting 'records': []).

peterthomassen · 2018-03-29T14:45:17Z

A few details about input validation for bulk requests: There are three stages during which exceptions can occur:

serializer.is_valid(): Those are errors like missing JSON fields, negative TTL and such (based on RRset model metadata).
Uniqueness. This is tested in the run-up of the database write operation, in RRsetSerializer._save(). Here, we also track that one bulk request does not contain two orders for the same (subname, type).
Lastly, the type or record contents may be deemed invalid by pdns. This only occurs after the tentative database write (before committing transaction).

Errors are collected in each stage and presented as a list of errors, with each list item referring to one part of the bulk request, in the same order. Parts that did not cause errors have {}, and parts with errors have a dict describing the error. Only in step 3 it is not possible to distinguish by bulk part, as the pdns response does not contain information on which RRset was faulty.

The treatment of stages 1 and 2 may mean that one bulk part appears without an error in stage 1 ({}) if another part has an error. Only after this error is resolved, the request will reach stage 2, at which point an error may occur for the bulk part that previously seemed valid.

While it would be possible to merge stages 1 and 2, it would rip apart the usual structure of DRF serializers, and I'm also not sure if it is worth the effort. It probably suffices to mention this behavior in the docs.

Errors in stages 1 and 2 cause status code 400 (regardless of the exact reason(s) which may vary across bulk parts), and errors from stage 3 cause status code 422.

peterthomassen · 2018-03-29T17:21:12Z

Outstanding issues:

test bulk-messing with SOA and other restricted types
PATCHing [{"subname": "d.1", "ttl": 50, "type": "AAA"}] does not behave correctly (weird type)
PATCH generally does not require the ttl field, but how about new RRsets with PATCH? And at least for delection (records = []), ttl should not be required.
docs

Not aware of anything else.

peterthomassen · 2018-04-03T20:45:53Z

All done except docs, which I prefer to write once the bulk API has been reviewed.

@nils-wisiol Can you look at this PR when you find some time? Thanks!

nils-wisiol · 2018-04-07T04:45:37Z

api/desecapi/settings.py

@@ -181,7 +181,7 @@
    DEBUG = True
    EMAIL_BACKEND = 'django.core.mail.backends.dummy.EmailBackend'
    ABUSE_BY_REMOTE_IP_LIMIT = 100
-    ABUSE_BY_REMOTE_IP_PERIOD_HRS = 4800
+    ABUSE_BY_REMOTE_IP_PERIOD_HRS = 2


To disable the limit, this should be set to zero.

Yes, but do we want to disable the limit? We may want to have the functionality available.

I'd say disable it for now, there are no tests that require the limit. It's better to have it definitely disabled than to have it go off at 100 different email addresses, causing random e2e test failures (as the order of the requests is not deterministic).

Ok. I'll also add a pending test for user locking.

nils-wisiol · 2018-04-07T04:45:43Z

api/desecapi/settings.py

    ABUSE_BY_EMAIL_HOSTNAME_LIMIT = 100
-    ABUSE_BY_EMAIL_HOSTNAME_PERIOD_HRS = 2400
+    ABUSE_BY_EMAIL_HOSTNAME_PERIOD_HRS = 1


To disable the limit, this should be set to zero.

Yes, but do we want to disable the limit? We may want to have the functionality available.

nils-wisiol · 2018-04-07T05:11:14Z

api/desecapi/models.py

@@ -290,6 +315,11 @@ def write_rrsets(self, rrsets):
        if rrsets_to_write and not self.owner.locked:
            pdns.set_rrsets(self, rrsets_to_write)

+        # Return RRsets written
+        return [rrsets_return_lookup[rrset]


I appreciate the extension to write_rrsets! I still believe though that this method is too long and complicated for a throughout review -- I advise a refactoring in the near future, splitting functionality in several subroutines. For now, I can approve this based on a general understanding of what's going on and based on the passing tests. (#97)

nils-wisiol · 2018-04-07T05:28:37Z

test/e2e/spec/api_spec.js


                it("can register a domain name", function () {
                    var domain = 'e2etest-' + require("uuid").v4() + '.dedyn.io';
                    return expect(chakram.post('/domains/', {'name': domain})).to.have.status(201);
                });

+                it("has correct JSON schema");


unnecessary/missing function() { argument?

No. That's a "pending test" (neither failing nor passing); shows up with "-" instead of "x" or checkmark in the results. It's meant to say that this test has no implementation yet.

nils-wisiol · 2018-04-07T05:31:28Z

test/e2e/spec/api_spec.js

+                    it("with correct status and schema", function () {
+                        var response = chakram.get('/domains/' + domain + '/rrsets/');
+                        expect(response).to.have.status(200);
+                        expect(response).to.have.schema(rrsetsSchema);


This test will always result in an empty rrsets reply. Although correct, is that the point?

No, the NS RRset should be there. I'll add a check for it to the test.

nils-wisiol · 2018-04-11T06:47:22Z

Before we merge this, I'd like to add further e2e tests. To that end I updated the e2e dns test interface to use a different dns library, so that dns responses include more details (most importantly, the TTL). Also, it's quite easy to add further RR set types.

Example usage of the new code:

describe("a domain endpoint", function () {

    var domain = 'e2etest-' + require("uuid").v4() + '.dedyn.io';

    before(function () {
        return expect(chakram.post('/domains/', {'name': domain})).to.have.status(201);
    });

    function postAndPropagationTest(subname, domain, type, records, ttl) {
        var queryName = subname ? subname + '.' + domain : domain;
        return describe('posting ' + type + ' records at ' + queryName, function () {
            before(function () {
                return expect(chakram.post(
                    '/domains/' + domain + '/rrsets/',
                    {
                        'subname': subname,
                        'type': type,
                        'records': records,
                        'ttl': ttl,
                    }
                )).to.have.status(201);
            });
            it('can be set', function(){});
            it('TTL propagates to pdns', function () {
                return expect(chakram.resolve(queryName, type)).to.have.dns.ttl(ttl);
            });
            it('records propagate to pdns', function () {
                return expect(chakram.resolveStr(queryName, type)).to.have.members(records);
            });
            it('records propagate to pdns (reverse order)', function () {
                return expect(chakram.resolveStr(queryName, type)).to.have.members(records.reverse());
            });
        });
    }

    postAndPropagationTest('', domain, 'A', ['1.1.1.1', '2.2.2.2'], 42);
    postAndPropagationTest('a', domain, 'A', ['7.7.7.7', '8.8.8.8'], 41);
    postAndPropagationTest('', domain, 'AAAA', ['::1'], 77);
    postAndPropagationTest('a', domain, 'AAAA', ['bade::affe', 'dead::beef'], 71);
    postAndPropagationTest('', domain, 'MX', ['10 example.net.', '20 a.example.net.'], 60);
    // Note that this is not yet working as the TLSA decoding is missing from dns-packet.
    // postAndPropagationTest('', domain, 'TLSA',
    //     ['3 1 1 1306c288896771534a5b60744a535e1025f79b0b64e1bbc89278ba4d413cb9f1'], 120);

});

More RR set types can easily be added.

Note that to my best knowledge, .to.have.members() is order-insensitive (contrary to what you said). We should verify your claim before merging 6d97421.

I propose you cherry-pick commits d6e8371 and b7ce50c into your PR and adopt your e2e tests before we proceed. Note that the latter breaks existing test cases that rely on resolve(), as the API changed. I did not fix them as your PR moves them around and fixing it on master would just result in a big merge mess.

peterthomassen · 2018-04-11T16:50:14Z

Thanks for doing all that work! 👍

Note that to my best knowledge, .to.have.members() is order-insensitive (contrary to what you said). We should verify your claim before merging 6d97421.

I said that .members() is supposed to be order-insensitive, but in my experience with it, it was not, for unknown reasons. I'll be very happy if it turns out to actually work as advertised.

I propose you cherry-pick commits d6e8371 and b7ce50c into your PR and adopt your e2e tests before we proceed. Note that the latter breaks existing test cases that rely on resolve(), as the API changed. I did not fix them as your PR moves them around and fixing it on master would just result in a big merge mess.

Sounds good.

peterthomassen · 2018-04-11T23:15:18Z

I cherry-picked as you proposed and refactored all tests to use the new DNS tool. I also added implementations for all tests that were pending (i.e. they had a name only).

Note that to my best knowledge, .to.have.members() is order-insensitive (contrary to what you said). We should verify your claim before merging 6d97421.

I managed to fix things up so that this commit can go away (already dropped it in this branch). I am not sure how I messed up previously.

So, I think, we're pretty much done! I have left the new changes as individual commits so that you can easily review them, but before merging, we may want to squash some of them. Here's a list:

1519604 fix(api): correct abuse limits for test scenario
91fc62a fix(api): fix hostname-based abuse check
9959686 refactor(api): add method to convert dict-like RRsets to RRset objects
e60ebd8 feat(api): have write_rrsets() return all RRsets value (idempotence)
848b38d feat(api): bulk REST requests, closes Create endpoint for atomic multi-updates #83
cfb8740 fix(tests): adjust status codes when changing immutable fields
6d65311 feat(dev): disable pdns cache for e2e tests
2529177 fix(e2e): rewrote dns test interface
0cbe0af fix(e2e): remove unnecessary flags from DNS resolve code
bfdd77b fix(e2e): close socket after tests so that mocha will exit
33038c5 fix(e2e): allow NXDOMAIN DNS response code
03f013f fix(e2e): add TXT record serialization to DNS toolset
88d4c76 fix(e2e): augment TTL test method
9dba795 feat(e2e): add test routine for checking RRsets in the DNS
3d04245 refactor(e2e): adapt dynDNS tests to use new DNS tool
a3e0c33 refactor(e2e): move API tests to api_spec.js
3630405 feat(tests): bulk API e2e tests
011b34d refactor(e2e): adapt API tests to use new DNS tool, add pending tests
b0f7b55 fix(e2e): correctly do record comparisons
79f1ce5 fix(api): improve validation of donation input data
734d7a6 fix(api): when PATCH'ing, correctly inform about missing type field
593803f fix(api): dummy TTL for RRsets up for deletion
f15b20e fix(api): disallow tinkering with OPT RRset
d146089 fix(api): disallow generic type format like TYPExxx
5b9b6d2 fix(api): fix deadlock by rephrasing WHERE clause
c9a4f5b refactor(api): simplify and clarify in Domain.write_rrsets()

I think it will be reasonable to squash 9 -- 13, 14--15, 17--19.

Also, as write_rrsets() has undergone a bunch of changes, it is unclear why two of them should be their own commits, so I'd suggest to squatch 4, 22, 26.

Let me know what you think, both about the status of the PR as well as the squashing!

nils-wisiol · 2018-04-13T00:57:57Z

yeah, please squash as proposed. Otherwise I'm afraid I'll get caught up in commenting on outdated changes :-)

peterthomassen · 2018-04-13T14:51:43Z

done!

nils-wisiol

I think commits 95ca54d and 48cbfdc should be together in one commit.

The e2e tests 5fa6878 have lots of duplicate code. I know that's hardly avoidable, but let's try to reduce it just a little: maybe some of the it("propagates to the API", function () { can be refactored in a similar manner like you did with itShowsUpInPdnsAs?

Also, I'm a fan of using e2e tests as API documentation, too. In that case, it may be necessary to re-order the tests. What's a good structure for such documentation? It could be organized by endpoint, then by method, like

/domain
- POST
- GET
- DELETE
/domain/{name}/rrsets/
- GET
/domain/{name}/rrsets/{subname}/{type}
- POST
  - single
  - bulk
- GET

and so on; or it could be organized like the current documentation on desec.io, i.e. by use case.

In any event, some documentation for this bulk features is still missing from this PR.

Other than a couple of small notes, I did not discover significant problems with this, and I'd like to see it in master soon.

To be entirely honest, I'm not too familiar with the bulk features of the rest framework, so that part of the code I did not review in great detail, but more based on the the tests and common sense.

Awesome work, thanks!

nils-wisiol · 2018-04-14T06:12:47Z

api/desecapi/settings.py

@@ -181,7 +181,7 @@
    DEBUG = True
    EMAIL_BACKEND = 'django.core.mail.backends.dummy.EmailBackend'
    ABUSE_BY_REMOTE_IP_LIMIT = 100
-    ABUSE_BY_REMOTE_IP_PERIOD_HRS = 4800
+    ABUSE_BY_REMOTE_IP_PERIOD_HRS = 2


I'd say disable it for now, there are no tests that require the limit. It's better to have it definitely disabled than to have it go off at 100 different email addresses, causing random e2e test failures (as the order of the requests is not deterministic).

nils-wisiol · 2018-04-14T06:14:56Z

api/desecapi/models.py

@@ -7,6 +7,7 @@
 import datetime, uuid


I did not understand what you meant with 2.) dummy TTL for RRsets up for deletion commit message

If you send a PATCH request with empty record contents [], the RRset is supposed to be deleted. This works by sending a similar requests with an empty records field to pdns. However, pdns requires the ttl field to be set, but our PATCH request reasonably does not require that. In this case, we set a dummy value.

(If you leave our the records field from our PATCH request, the RRset is not deleted; you can use such requests to change the TTL only.)

nils-wisiol · 2018-04-14T06:36:29Z

test/e2e/setup.js

+        chakram.expect(chakram.resolveStr(queryName, type)).to.have.lengthOf(records.length);
+        chakram.expect(chakram.resolveStr(queryName, type)).to.have.members(records);
+        chakram.expect(chakram.resolveStr(queryName, type)).to.have.members(records.reverse());
+        if (typeof ttl !== "undefined") {


(no changes required, just commenting on javascript) typeof undefined === 'undefined' 😳

nils-wisiol · 2018-04-14T06:40:14Z

test/e2e/setup.js

@@ -140,6 +140,7 @@ chakram.resolveStr = function (name, type) {
            case 'A': return data;
            case 'AAAA': return data;
            case 'MX': return data.preference + ' ' + data.exchange + '.';
+            case 'TXT': return '"' + data + '"';


why put quotes around it? Quotes in the middle will not be escaped anyways

nils-wisiol · 2018-04-14T06:46:19Z

test/e2e/spec/api_spec.js

-                    var domain = 'e2etest-' + require("uuid").v4() + '.dedyn.io';
+                    var response = chakram.get('/domains/' + domain + '/');
+                    expect(response).to.have.status(200);
+                    //expect(response).to.have.schema(domainSchema);


why not use that one? Remove and restructure this it, or add it. (For one expectation, we don't need chakram.wait()). Also, it's a misnomer

nils-wisiol · 2018-04-14T06:55:36Z

test/e2e/spec/api_spec.js

+                        before(function () {
+                            var response = chakram.delete('/domains/' + domain + '/rrsets/delete-test.../A/');
+                            expect(response).to.have.status(204);
+                            return chakram.wait();


return expect is better (I believe), as it reduces the amount of promises we need to wait for

nils-wisiol · 2018-04-14T07:10:44Z

api/desecapi/views.py

-            raise ex
+        except ValidationError as e:
+            if isinstance(e.detail, dict):
+                all = e.detail.get('__all__')


shadows built-in all function

1.) have write_rrsets() return all RRsets value (idempotence) Previously, only new or changed RRsets that were written to pdns had been returned. Now, all RRsets that exist at the end of the operation are returned (i.e. only the deleted ones are missing). 2.) dummy TTL for RRsets up for deletion 3.) fix deadlock by rephrasing WHERE clause for RR deletion 4.) refactor: simplify and clarify Domain.write_rrsets()

This commit also slightly shortens certain cache times for production, from default to 20s.

This commits refactors chakram.resolve() and introduces chakram.resolveStr(), both return promises that will eventually be resolved into response data from nslord. It also introduces dns, a dummy property to increase readability of expectations and ttl(exp), a function to check if a RR set has the correct ttl.

1.) remove unnecessary flags from DNS resolve code The following flags (i.e. all) were removed: - Recursion Desired: we don't want nslord to recurse - Authoritative Answer: reserved for responses, see RFC 1035 Sec. 4.1.1 2.) close socket after tests so that mocha will exit 3.) allow NXDOMAIN DNS response code This is a valid response code when looking for a non-existing RRset on a subdomain that has no records at all. 4.) add TXT record serialization to DNS toolset 5.) augment TTL test method

peterthomassen · 2018-04-19T16:43:21Z

I think commits 95ca54d and 48cbfdc should be together in one commit.

Really? They have nothing to do with each other. Why do you think so?

The e2e tests 5fa6878 have lots of duplicate code. I know that's hardly avoidable, but let's try to reduce it just a little: maybe some of the it("propagates to the API", function () { can be refactored in a similar manner like you did with itShowsUpInPdnsAs?

Done, see c13f49d. I added this as a new commit for ease of review, but before merging, we should squash it into the main bulk e2e test commit.

I also created another commit that adds testing of the JSON structure of responses of the domain/ endpoint. Same goes for squashing before merge.

Also, I'm a fan of using e2e tests as API documentation, too. In that case, it may be necessary to re-order the tests. What's a good structure for such documentation? It could be organized by endpoint, then by method

I agree that that would be nice to have, but I think we should open an issue for this instead of further holding up this PR.

In any event, some documentation for this bulk features is still missing from this PR.

Yes. As mentioned in one of the earlier comments, I postponed writing it until it is clear (after your review) that the interface will not change much. (Otherwise, the whole bulk docs would have had to be rewritten.)

I'm working on this now, and I'd like to merge it in a separate PR. Are you ok with that?

Other than a couple of small notes, I did not discover significant problems with this, and I'd like to see it in master soon.

Nice & thanks! I added replies to your code comments fixed some minor issues you pointed out. There are no outstanding issues from these comments.

Pls let me know about any show-stoppers that I may have forgotten. Thanks!

peterthomassen · 2018-04-20T19:12:00Z

Just added the docs. A rendered version will be available for a few days at https://a4a.de/.u/1524251474_index.html

peterthomassen · 2018-04-20T20:09:13Z

@nils-wisiol Here's a question regarding the docs. So far, our examples all have HTTPie commands because that tool doesn't require the user to know how to construct JSON objects. For the new bulk requests, however, HTTPie doesn't have a useful interface (see httpie/cli#121), so I added examples with curl.
Our supporter @gerhard-tinned proposed long ago (in an email) that we should always show curl examples, and maybe now is the time to do that. Do you think we should get rid of the HTTPIe examples, then?

Ideally, it would be nice to add tabs to the examples to switch between the styles, but I don't want to spend the time to set this up. (Right now, we simply use rst2html5.py which is very straightforward).

gerhard-tinned · 2018-04-21T11:56:38Z

@peterthomassen For me, the example section of an api documentation is one of the most important part to start. It usually allows you an very easy start into whatever you try to do.

As curl is kind of a standard for testing and checking web services, my opinion is - curl is the must have in an api documentation.

If you add more options, ... even better. I would not remove the httpie commands. But I understand that maintaining both might be a bit much.

nils-wisiol · 2018-05-03T09:28:01Z

I extracted outstanding issues into

Add curl examples to docs Add curl examples to docs #99
add e2e tests to docs add e2e tests to docs #100
Clarify docs: all domain names need to end with a dot . Clarify docs: all domain names need to end with a dot . #101

I'm working on this now, and I'd like to merge it in a separate PR. Are you ok with that?

is there more to come?

Anyhow, I'm ready to approve this. lgtm! :)

nils-wisiol

don't forget the squash the last two commits!

peterthomassen · 2018-05-03T09:30:05Z

Wow. Thanks for the review!

I'm working on this now, and I'd like to merge it in a separate PR. Are you ok with that?

is there more to come?

Not for the moment.

don't forget the squash the last two commits!

Shall do, thanks for the reminder!

Previously, IBAN and BIC were normalized and stripped from whitespace only for creating the direct debit attachment file. If the user entered spaces like ' DE123455....', those were stored in the database. Along with the 6-character cutoff, this was not desirable. Space stripping is now done in the Donation serializer so that it applies to both what's stored and to what's put in the direct debit file. Also, e2e tests were adapted to verify the presence of the masked IBAN.

OPT is a reserved record type with special meaning and is not supposed to exist in a zone. Currently, if an OPT RRset is created, the zone may break. Until PowerDNS ensures that RRsets of type OPT cannot created, let's make sure on this level. PowerDNS issue: PowerDNS/pdns#6441

pdns would convert types like TYPE99 to the named ones (here: SPF), but our API cannot do so without maintaining a mapping table that depends on the pdns version. Currently, we would store two distinct RRsets in our API database, corresponding to only one RRset in pdns. Worse, if one RRset was deleted (along with the one on pdns), the other would remain as an orphan. The only usecase for this would be new types quickly gaining popularity although pdns does not know them yet. In this case, we can address the issue again.

peterthomassen added enhancement api labels Mar 29, 2018

peterthomassen self-assigned this Mar 29, 2018

peterthomassen requested a review from nils-wisiol March 29, 2018 02:27

peterthomassen force-pushed the 20180208_bulk branch from d970892 to 79ecccf Compare March 29, 2018 13:47

peterthomassen force-pushed the 20180208_bulk branch 4 times, most recently from 501b4a1 to 2b18fff Compare April 3, 2018 20:43

peterthomassen force-pushed the 20180208_bulk branch 2 times, most recently from c731587 to 4f3f8af Compare April 4, 2018 15:37

nils-wisiol reviewed Apr 11, 2018

View reviewed changes

peterthomassen force-pushed the 20180208_bulk branch 6 times, most recently from eb8293f to c9a4f5b Compare April 11, 2018 23:03

peterthomassen force-pushed the 20180208_bulk branch from c9a4f5b to abd8509 Compare April 13, 2018 14:45

nils-wisiol reviewed Apr 14, 2018

View reviewed changes

fix(api): fix hostname-based abuse check

c335b93

peterthomassen and others added 9 commits April 19, 2018 13:37

refactor(api): add method to convert dict-like RRsets to RRset objects

7bd585d

feat(api): bulk REST requests, closes #83

2c3cb67

fix(tests): adjust status codes when changing immutable fields

18f9dc8

feat(dev): disable pdns cache for e2e tests

bab39a4

This commit also slightly shortens certain cache times for production, from default to 20s.

feat(e2e): introduce new test routine for checking RRsets in the DNS

c312cad

refactor(e2e): move API tests to api_spec.js

5e842fb

peterthomassen force-pushed the 20180208_bulk branch from abd8509 to c13f49d Compare April 19, 2018 16:02

peterthomassen force-pushed the 20180208_bulk branch from 293969b to 1370e06 Compare April 20, 2018 19:39

nils-wisiol approved these changes May 3, 2018

View reviewed changes

peterthomassen added 8 commits May 3, 2018 11:33

feat(tests): bulk API e2e tests

1d03942

fix(api): relax abuse limits for test scenario

9158273

fix(api): when PATCH'ing, correctly inform about missing type field

dc64f00

feat(docs): add description of bulk features

9a09fd6

fix(docs): clarify that NS record values end with a dot

8060ac3

peterthomassen force-pushed the 20180208_bulk branch from 0ecf249 to 8060ac3 Compare May 3, 2018 09:33

peterthomassen merged commit ca8df4b into master May 3, 2018

peterthomassen deleted the 20180208_bulk branch May 3, 2018 09:40

bulk REST requests #96

bulk REST requests #96

Conversation

peterthomassen commented Mar 29, 2018 • edited

peterthomassen commented Mar 29, 2018

peterthomassen commented Mar 29, 2018

peterthomassen commented Mar 29, 2018 • edited

peterthomassen commented Apr 3, 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

peterthomassen Apr 11, 2018 • edited

Choose a reason for hiding this comment

nils-wisiol commented Apr 11, 2018 • edited

peterthomassen commented Apr 11, 2018 • edited

peterthomassen commented Apr 11, 2018

nils-wisiol commented Apr 13, 2018

peterthomassen commented Apr 13, 2018

nils-wisiol 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

peterthomassen commented Apr 19, 2018

peterthomassen commented Apr 20, 2018

peterthomassen commented Apr 20, 2018

gerhard-tinned commented Apr 21, 2018

nils-wisiol commented May 3, 2018

nils-wisiol left a comment

Choose a reason for hiding this comment

peterthomassen commented May 3, 2018

peterthomassen commented Mar 29, 2018 •

edited

peterthomassen commented Mar 29, 2018 •

edited

peterthomassen Apr 11, 2018 •

edited

nils-wisiol commented Apr 11, 2018 •

edited

peterthomassen commented Apr 11, 2018 •

edited