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

Mapper proposal #2147

Merged
merged 7 commits into from
Nov 5, 2020
Merged

Mapper proposal #2147

merged 7 commits into from
Nov 5, 2020

Conversation

sailorvii
Copy link
Contributor

What type of PR is this?
/kind design

What this PR does / why we need it:
Simplify the mapper structure

Special notes for your reviewer:

Does this PR introduce a user-facing change?:
All mappers will be refined.

@kubeedge-bot kubeedge-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 9, 2020
@luogangyi
Copy link
Member

@kevin-wangzefeng @fisherxu let's discuss it in next community meeting

@fisherxu fisherxu changed the title Mapperproposal Mapper proposal Sep 10, 2020
@kevin-wangzefeng kevin-wangzefeng added this to the v1.5 milestone Sep 23, 2020
The original description could refer to the doc [mapper-design.md](https://github.com/kubeedge/kubeedge/blob/master/docs/proposals/mapper-design.md "mapper-design.md").

Mapper is an interface between kubeedge and devices. It could set/get device data, get and report the device status.
Kubeedge uses device controller, device twin and mapper to control the devices.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Kubeedge uses device controller, device twin and mapper to control the devices.
KubeEdge uses device controller, device twin and mapper to control the devices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review and time.

Done.

Comment on lines 7 to 10
approvers:
- "@"
- "@"
creation-date: 2020-09-
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
approvers:
- "@"
- "@"
creation-date: 2020-09-
approvers:
- "@kevin-wangzefeng"
- "@"
creation-date: 2020-09-09

Comment on lines 133 to 138
const DeviceStatus {
DEVSTOK
DEVSTERR /*Expected value is not equal as setting*/
DEVSTDISCONN
DEVSTUNHEALTHY /*Unhealthy status from device*/
}
Copy link
Member

Choose a reason for hiding this comment

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

Not quite understand the code here, what's the value type of the consts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just want to present all status that reported to the device controller. In the code, the type is uint.

@kevin-wangzefeng
Copy link
Member

Minor comments, thanks

@kevin-wangzefeng
Copy link
Member

/approve

@kubeedge-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kevin-wangzefeng

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubeedge-bot kubeedge-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 29, 2020
@kevin-wangzefeng
Copy link
Member

/assign @fisherxu

@kevin-wangzefeng
Copy link
Member

nit: maybe "mapper refactoring proposal" or "mapper design v2" is better for the proposal name

This was referenced Oct 26, 2020
@fisherxu
Copy link
Member

@sailorvii Let's rename the proposal to "mapper design v2"?

Copy link
Member

@fisherxu fisherxu left a comment

Choose a reason for hiding this comment

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

Also please squash the commits :)

docs/proposals/mapper-design-v2.md Outdated Show resolved Hide resolved
@kubeedge-bot kubeedge-bot merged commit 1472849 into kubeedge:master Nov 5, 2020
@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants