Validate trigger target on create/update #817
Conversation
Dimedrolity
force-pushed
the
feature/trigger-validation-v2
branch
6 times, most recently
from
b9eff75
to
32ba248
Compare
Dimedrolity
force-pushed
the
feature/trigger-validation-v2
branch
from
32ba248
to
d1b8e79
Compare
|
Converted to draft because need to add validation on updating existing trigger |
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #817 +/- ##
==========================================
+ Coverage 70.32% 70.51% +0.18%
==========================================
Files 179 179
Lines 9695 9740 +45
==========================================
+ Hits 6818 6868 +50
+ Misses 2494 2484 -10
- Partials 383 388 +5
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Dimedrolity
force-pushed
the
feature/trigger-validation-v2
branch
2 times, most recently
from
a1fa5be
to
2879e69
Compare
Also rename TriggerCheckResponse to TriggerCheckResult, because it is not http Response, it is content of http response.
…t it. New version can be used with query param. It should not affect prev version, it is important for backward compatibility, because Moira is follows API first approach. Make ProblemOfTarget to use it in tests.
…t it. New version can be used with query param. It should not affect prev version, it is important for backward compatibility, because Moira is follows API first approach.
Maybe it is better to move this funcs to dto or controller pkgs.
Dimedrolity
force-pushed
the
feature/trigger-validation-v2
branch
from
2879e69
to
8c88712
Compare
Dimedrolity
force-pushed
the
feature/trigger-validation-v2
branch
from
8c88712
to
8278c59
Compare
Dimedrolity
changed the title
Also add comment to stress doubt, why errors are ignored.
| @@ -100,17 +100,17 @@ var ( | |||
| } | |||
| ) | |||
|
|
|||
| type problemOfTarget struct { | |||
| type ProblemOfTarget struct { | |||
There was a problem hiding this comment.
hmm, why become struct as public?
There was a problem hiding this comment.
This struct is used in tests when compare expected and actual
There was a problem hiding this comment.
only for tests? I think better leave struct private to avoid unexpected usage and make constructor....
There was a problem hiding this comment.
Yes, only for tests, tests are important part, it uses pkg like a public API. I think constructor for problemOfTarget is not need, because there is no init logic, so now it is fine.
Can we estimate unexpected usage of public ProblemOfTarget, would it lead to some problems?
There was a problem hiding this comment.
i think myself that public structs are evil, but ok, lets leave, and add godoc please)
There was wrong content-type for response, it was 'text/plain'. Now it is JSON. Use render pkg functions to set StatusCode and JSON payload. It is because `responseWriter.Result()` use `snapHeader` field, which is 'snapshot of HeaderMap at first Write', so write operations to response need to use via render pkg functions, not manually.
There is a problem that client sides should call /trigger/check/ API for validation, but obviously it is not necessary. As a result we have invalid triggers in DB. The plan consists of next steps:
Then in another PRs update client sides to use new version of APIs.