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

Fix dead link for bandwidth plugin comment #79

Closed
wants to merge 1 commit into from

Conversation

MikeZappa87
Copy link
Contributor

Resolving a dead link for the bandwidth plugin.

Resolves Issue:
#78

@codecov-commenter
Copy link

codecov-commenter commented Dec 24, 2021

Codecov Report

Merging #79 (4f53315) into main (aa8bf14) will increase coverage by 1.68%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #79      +/-   ##
==========================================
+ Coverage   42.23%   43.91%   +1.68%     
==========================================
  Files           9        9              
  Lines         367      378      +11     
==========================================
+ Hits          155      166      +11     
  Misses        181      181              
  Partials       31       31              
Impacted Files Coverage Δ
namespace_opts.go 0.00% <ø> (ø)
cni.go 74.03% <0.00%> (+3.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa8bf14...4f53315. Read the comment docs.

@@ -34,7 +34,7 @@ func WithCapabilityIPRanges(ipRanges []IPRanges) NamespaceOpts {
}

// WithCapabilityBandWitdh adds support for traffic shaping:
// https://github.com/heptio/cni-plugins/tree/master/plugins/meta/bandwidth
// https://github.com/containernetworking/plugins/tree/master/plugins/meta/bandwidth
Copy link
Member

Choose a reason for hiding this comment

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

Does this plugin work correctly today with this library?

There seems to be a mismatch in the json fields

https://github.com/containernetworking/plugins/blob/master/plugins/meta/bandwidth/main.go#L40

and

https://github.com/containerd/go-cni/blob/main/types.go#L50

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this referring to the missing json annotations in the types.go file? I am tracing the code path in the cni,go-cni and contained repos.

Copy link
Member

Choose a reason for hiding this comment

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

right should check if all is well after we changed it from int to uint64 here in go-cni.. but kept pointing to hepatio... as a service question

@estesp
Copy link
Member

estesp commented Apr 18, 2022

needs a rebase

@estesp
Copy link
Member

estesp commented Oct 27, 2022

@MikeZappa87 if this is still valid, we should get this in..looks like it fell between the cracks

@MikeZappa87
Copy link
Contributor Author

Maybe I should get this done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants