Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MTU field to route type definition #1005

Closed
wants to merge 4 commits into from

Conversation

Joffref
Copy link

@Joffref Joffref commented Jul 4, 2023

Fixes: #1004

pkg/types/020/types_test.go Outdated Show resolved Hide resolved
@aojea
Copy link

aojea commented Jul 4, 2023

have you tested this? it does not look like #831

It also required to update Spec.md

/cc @squeed @dcbw

@@ -53,14 +53,14 @@ func testResult(resultCNIVersion, jsonCNIVersion string) (*types020.Result, stri
IP: *ipv4,
Gateway: net.ParseIP("1.2.3.1"),
Routes: []types.Route{
{Dst: *routev4, GW: routegwv4},
{Dst: *routev4, GW: routegwv4, MTU: 1024},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably not add any new fields to v0.2.0 and v0.4.0 -- what do you think, @dcbw ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I remove new fields inside v0.2.0 and v0.4.0, while updating SPEC.md?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Joffref yes please.
(Sorry, I somehow missed your question!)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change made!

@squeed
Copy link
Member

squeed commented Aug 14, 2023

This looks basically good. We also need to describe this in SPEC.md. Shouldn't be more than a line or two needed.

@@ -17,13 +17,13 @@ package types020_test
import (
"encoding/json"
"fmt"
types020 "github.com/containernetworking/cni/pkg/types/020"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit this change same unrelated

@Joffref
Copy link
Author

Joffref commented Dec 19, 2023

Added in #1041

@Joffref Joffref closed this Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add MTU as a Route property
3 participants