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

Make MTU optional for VRFrouteconfiguration #138

Merged
merged 2 commits into from
Aug 15, 2024
Merged

Make MTU optional for VRFrouteconfiguration #138

merged 2 commits into from
Aug 15, 2024

Conversation

watzkuh
Copy link
Collaborator

@watzkuh watzkuh commented Aug 13, 2024

No description provided.

@schrej
Copy link
Member

schrej commented Aug 13, 2024

Is this safe to just omit?

Took a brief look at the code, and at least in one location defaulting doesn't seem to be properly implemented: https://github.com/telekom/das-schiff-network-operator/blob/main/pkg/nl/layer3.go#L191

@watzkuh
Copy link
Collaborator Author

watzkuh commented Aug 14, 2024

I'm not entirely sure. I'm only assuming it because from what I can see we have the value set for only 3 of our own VRFRouteConfigurations and unset for 816 of them* and I was hoping they weren't all broken.

(*assuming I greped correctly)

@chdxD1
Copy link
Member

chdxD1 commented Aug 15, 2024

@schrej good catch, that line should also contain info.linkMTU() however it had no impact so far

@chdxD1
Copy link
Member

chdxD1 commented Aug 15, 2024

@watzkuh If you could change the line mentioned by Jakob from info.MTU to info.linkMTU I approve this PR

@chdxD1 chdxD1 self-assigned this Aug 15, 2024
@watzkuh
Copy link
Collaborator Author

watzkuh commented Aug 15, 2024

@chdxD1 Done!

Copy link
Member

@chdxD1 chdxD1 left a comment

Choose a reason for hiding this comment

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

lgtm

@chdxD1 chdxD1 merged commit 9281253 into main Aug 15, 2024
3 checks passed
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.

3 participants