-
Notifications
You must be signed in to change notification settings - Fork 21
Document the need for COSI, and Developer guide #22
Conversation
@xing-yang please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
If you are new to the SIG Storage COSI project, check out the [spec](https://github.com/kubernetes-sigs/container-object-storage-interface-spec/blob/master/spec.md), [KEP](https://github.com/kubernetes/enhancements/tree/master/keps/sig-storage/1979-object-storage-support), and [project board](https://github.com/orgs/kubernetes-sigs/projects/). | ||
```sh | ||
# cleans up old build files | ||
make clobber |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why make clobber? can make clean do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is in addition to make clean
as there are some intermediate files that need to be cleaned up while keeping the generated protobuf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah
/lgtm |
|
||
## Why another standard? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section is a dup from another doc. Better to link to a single source here.
/approve |
README.md
Outdated
|
||
- Unit of provisioned storage - Bucket instead of filesystem mount or blockdevice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blockdevice -> block device
README.md
Outdated
|
||
## Community, discussion, contribution, and support | ||
- Must be backwards compatible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is practical for alpha APIs? In Kubernetes, if API stays in alpha, it does not need to support backward compatibility; but once it reaches beta, it has to support backward compatibility moving forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a practical guideline for the community contributors who want to start contributing. I think between us in the team, we can have an understanding about it until we reach v1 status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the language in the spec repo is supposed to be precise. We can't require alpha APIs to be backward compatible. Alpha means it is experimental. If this requirement is added here but we are not really going to enforce it, then we should either loosen the requirement or remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense. Ive updated the docs
|
||
## Community, discussion, contribution, and support | ||
- Must be backwards compatible | ||
- Must be in-sync with the API definitions in [sigs.k8s.io/container-object-storage-interface-api](https://github.com/kubernetes-sigs/container-object-storage-interface-api) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
COSI spec is a lower level API. I think it should not talk about the higher level API. It is the other way around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two are closely inter-related. we want to move them in lockstep.i.e. If a change is made here, then it should also be made in api.
@xing-yang I've addressed your comments |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brahmaroutu, tparikh, wlan0, xing-yang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This is ready for review
#14