chore: feedback for @maurafortino 's #587 PR #591
Conversation
denopink
force-pushed
the
denopink/feat/feedback-for-webhook-v2-branch
branch
2 times, most recently
from
55e79a2 to
b3a7a34
Compare
denopink
force-pushed
the
denopink/feat/feedback-for-webhook-v2-branch
branch
from
b3a7a34 to
4ca4e17
Compare
|
Went over the code with marua on a slack call. We'll merge this either today or tomorrow morning. |
| case *ancla.RegistryV2: | ||
| if len(l.Registration.Kafkas) == 0 && len(l.Registration.Webhooks) == 0 { |
There was a problem hiding this comment.
would it make sense to add this to the validation portion of web hook-schema? just thinking about ways to try and clean up the this switch statement
There was a problem hiding this comment.
Yeah, I think.
I suggest leaving it here until we successfully move it cleanly
| } | ||
| if len(wh.ReceiverURLs) == 0 && len(wh.DNSSrvRecord.FQDNs) == 0 { |
| } | ||
|
|
||
| transport, err := NewSRVRecordDailer(wh.DNSSrvRecord) |
There was a problem hiding this comment.
I know it was already mentioned but just making a note so we remember to change this to NewSRVRecordDialer
| } | ||
|
|
||
| if v2.client != nil { | ||
| resp, err = v2.client.Do(req) | ||
| } else { |
There was a problem hiding this comment.
do we want this else statement? are we not trying to have retry logic for the custom client that we have?
There was a problem hiding this comment.
yeah, that's exactly what I want to do. i.e.: use the retry client with a given dailer (the dailer may be custom or be default).
Either of us can do that. If you're busy, I can knock that out tomorrow morning.
There was a problem hiding this comment.
i should be able to work on this
internal/sink/srvRecordDailer.go
Outdated
| return d.srvs[i].Priority < d.srvs[j].Priority | ||
| }) | ||
| default: | ||
| return nil, fmt.Errorf("unknwon loadBalancingScheme type: %s", d.dnsSrvRecord.LoadBalancingScheme) |
There was a problem hiding this comment.
spelling change: unknown
internal/sink/sink.go
Outdated
| } | ||
| return nil | ||
|
|
||
| return nil, fmt.Errorf("unknow webhook registry type") |
There was a problem hiding this comment.
spelling change: unknown
No description provided.