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 documentation on the expectations of vendor images. #535

Merged
merged 3 commits into from
May 20, 2024
Merged

Conversation

bormanp
Copy link
Collaborator

@bormanp bormanp commented May 6, 2024

Add documentation on the expectations of vendor images.

@bormanp bormanp requested a review from alexmasi May 6, 2024 16:23
@coveralls
Copy link

coveralls commented May 6, 2024

Pull Request Test Coverage Report for Build 9128632529

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 137 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.1%) to 65.251%

Files with Coverage Reduction New Missed Lines %
topo/topo.go 29 78.29%
topo/node/arista/arista.go 54 71.64%
topo/node/node.go 54 31.54%
Totals Coverage Status
Change from base Build 8840821359: 0.1%
Covered Lines: 4640
Relevant Lines: 7111

💛 - Coveralls

docs/vendor.md Outdated
Comment on lines 81 to 85
Vendors are responsible for support of their images. This includes the node
implementation in
[kne/topo/node](https://github.com/openconfig/kne/tree/main/topo/node) as well
as the vendor specific examples in
[kne/examples](https://github.com/openconfig/kne/tree/main/examples).
Copy link

Choose a reason for hiding this comment

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

Are community contributions to vendor node implementations welcome? Do you expect or require vendors to provide review on community contributions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think community contributions should be welcome though the vendor should be the one to approve changes to code they are responsible for. I will updated the document.

Thanks!

Copy link

Choose a reason for hiding this comment

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

I agree in principle, but I would caution about being too dogmatic about this. For example, I have a vendor who is not inclined to provide support for one containerized platform of theirs in KNE; I have local (not ready to publish, but do intend to) modifications to provide that support and I would be concerned if I seemed there was limited hope of this being accepted upstream given the vendors' apparent reticence.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the question will come down to who has taken the responsibility to maintain that code. If the ACME Network Switch company has provided a node implementation (implying they are the maintainer of that code) for most of their switches and you want to add in code to support one of the missing switches you have two paths. The best option is to provide a PR that updates their node implementation and get their approval (as they maintain that node implementation). If that path is not possible there can always be a new node implementation offered, though the benefits of this would need to out weigh the costs and technical debt. If a the maintainer of a node implementation is non-response then you could take over maintenance of that code.

…ository

are welcome and that the vendors should review and approve those PRs.
docs/vendor.md Outdated
to be used with KNE.

This document describes the image requirements and expectations in order to be
considered *KNE Qualified*.
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does KNE qualified mean in this case? Will there be an indicator anywhere that the image is KNE qualified, as in, is this the requirements for us to merge a new node impl? Or do mark some of the node impls as "qualified"?

docs/vendor.md Outdated
## Testing

Vendor images must be tested prior to publication. A standard set of tests is
found at <INSERT LOCATION HERE>. It is expected that the image undergoes
Copy link
Collaborator

Choose a reason for hiding this comment

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

which tests are intended here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure these tests yet exist, though at a minimum KNE when the image is used bringing up KNE should not hang or crash because of the image, nor prevent other images from being used.

@bormanp
Copy link
Collaborator Author

bormanp commented May 17, 2024

@chrisy @alexmasi PTAL

@bormanp bormanp merged commit 0e17b04 into main May 20, 2024
10 checks passed
@bormanp bormanp deleted the borman-vendor branch May 20, 2024 23:08
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.

None yet

4 participants