[networks]: add decoder/encoder to Attachment and NetworkStatus#1216
[networks]: add decoder/encoder to Attachment and NetworkStatus#1216saehejkang wants to merge 3 commits intoapple:mainfrom
Conversation
Co-authored-by: Ed Saipetch <ed@twentybelow.com>
saehejkang
force-pushed
the
add-init-decoder-for-attachment-and-network-config
branch
from
96a7b48 to
16eb09b
Compare
|
|
||
| network = try container.decode(String.self, forKey: .network) | ||
| hostname = try container.decode(String.self, forKey: .hostname) | ||
| if let address = try? container.decode(CIDRv4.self, forKey: .ipv4Address) { |
There was a problem hiding this comment.
Suggestion: would be nice to add some tests for legacy payload decoding.
There was a problem hiding this comment.
This might warrant an issue (bug or request) to be added on later. I want to get these changes merged soon because they are blocking socktainer users from using the latest version of container.
|
|
||
| network = try container.decode(String.self, forKey: .network) | ||
| hostname = try container.decode(String.self, forKey: .hostname) | ||
| if let address = try? container.decode(CIDRv4.self, forKey: .ipv4Address) { |
There was a problem hiding this comment.
Suggestion: I think optional try? for fallback approach will swallow decode errors. A better approach would be fallback only on keyNotFound and throw error for malformed payloads. You could use something akin to;
private static func decodeOrFallback<T: Decodable>(
_ type: T.Type,
from container: KeyedDecodingContainer<CodingKeys>,
primary: CodingKeys,
fallback: CodingKeys
) throws -> T {
do {
// return primary
} catch let DecodingError.keyNotFound(key, _) where key.stringValue ==
primary.stringValue {
// return fallback
} catch {
// throw error
}
}
// callsite will be much cleaner
ipv4Address = try Self.decodeOrFallback(
CIDRv4.self,
from: container,
primary: .ipv4Address,
fallback: .address
)There was a problem hiding this comment.
I see the point about being more precise with error handling, but in this case I think it is a bit over-engineered for what we need right now. I also agree that swallowing all decode errors via try? isn’t ideal, but at the moment we don’t have meaningful error propagation wired up (managed resource work). Would prefer to just keep what we have here.
|
@jglogan bringing this to your inbox as it a blocker for socktainer and should get merged soon |
3 participants
Type of Change
Motivation and Context
Adds backward-compatible JSON decoding/encoding fallback for
AttachmentandNetworkStatusto preventkeyNotFounderrors when communicating with oldercontainer-apiserverversions.Temporary fix but closes #1196
Testing
Tests ✅ based on comment here