14: skip unknown types #181
Conversation
Jorropo
force-pushed
the
fix-unexpected-types
branch
from
0492f72
to
204c5a5
Compare
| var newSwarm []interface{} | ||
| for _, v := range swarm { | ||
| if addr, ok := v.(string); ok { | ||
| if !strings.Contains(addr, "/quic") || strings.Contains(addr, "/quic-v1") { |
There was a problem hiding this comment.
Not sure if we have good options, but it's pretty awkward that we're removing /quic here when we have no idea if there's a substitute /quic-v1 in place. Should we look for identical matches instead, otherwise we're going to delete the users' data that they may have added to AppendAnnounce, etc.
Given the user has to do something here would we be better off:
- Changing their custom config to
/quic-v1 - Deleting
/quic - Letting the user get an error on start and figure it out (error messages in kubo need to be helpful enough for this to be viable, they might already be)
Seems like probably not a ton of people will be impacted, but the support time here might be no fun unless this is taken care of 😢.
There was a problem hiding this comment.
we already had a migration that duplicated quic into quic-v1 addresses, do you want me to copy paste it and rerun it again ?
There was a problem hiding this comment.
Well don't want to end up with duplicate quic-v1 addresses. Could do more work here to make sure we're not, but a bit concerned about added surface needing testing.
Might be fine leaving things alone given it meant that after quic-v1 addresses were in the config the user added only a /quic address.
WDYT?
There was a problem hiding this comment.
@lidel says the previous code did deduplication, if it does then lets reuse that.
There was a problem hiding this comment.
+1, it will be safer is we effectively re-run migration and convert all remaining /quic to /quic-v1 and then remove any /quic-1 duplicates.
This way we won't break users who manually added /quic since last migration for soem reason.
| if !ok { | ||
| continue | ||
| fmt.Printf("invalid type for .Addresses got %T expected json map; skipping .Addresses\n", a) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
What kinds of things are we expecting here? Is the idea that we don't really care, it's not something with a valid /quic address we can deal with?
There was a problem hiding this comment.
mb should be a continue
Per 2023-09-12 conversation: we should get tests so we're not relying on manual tests. Right now we support a single input/output test. This should be expanded to support multiple. |
|
About testing for |
There was a problem hiding this comment.
lgtm, fwiw i've tested manually the scenario described by Adin and works fine.
I think it's ok to ship, we've spent enough time on this already, but if we end up having to fix this migration one more time 🙃 then we will create a separate test for every edge case.
No description provided.