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.

Sign up for GitHub

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

Jump to bottom

Add mapper template and refine the make method. #41

Merged
merged 1 commit into from
Jun 1, 2021
Merged

Add mapper template and refine the make method. #41

merged 1 commit into from
Jun 1, 2021

Conversation

sailorvii
Copy link
Collaborator

What type of PR is this?
/kind enhancement

What this PR does / why we need it:
Make things easy to develop a new mapper.

Which issue(s) this PR fixes:
Fixes #14

@kubeedge-bot
Copy link
Collaborator

@sailorvii: The label(s) kind/enhancement cannot be applied, because the repository doesn't have them

In response to this:

What type of PR is this?
/kind enhancement

What this PR does / why we need it:
Make things easy to develop a new mapper.

Which issue(s) this PR fixes:
Fixes #14

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@kubeedge-bot kubeedge-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 29, 2021
@sailorvii sailorvii mentioned this pull request May 10, 2021
Closed
@sailorvii sailorvii force-pushed the temp branch 2 times, most recently from 03f5a2e to a4ce9e0 Compare May 11, 2021 08:43
fisherxu
fisherxu reviewed May 31, 2021
View reviewed changes
_template/mapper/cmd/main.go Show resolved Hide resolved
_template/mapper/configmap/parse_test.go Show resolved Hide resolved
_template/mapper/device/device.go Outdated Show resolved Hide resolved
_template/mapper/driver/client.go Show resolved Hide resolved
mappers/ble/driver/client.go Outdated Show resolved Hide resolved
mappers/opcua/cmd/main.go Outdated Show resolved Hide resolved
mappers/opcua/configmap/parse.go Outdated Show resolved Hide resolved
mappers/opcua/configmap/parse_test.go Outdated Show resolved Hide resolved
mappers/opcua/device/device.go Outdated Show resolved Hide resolved
mappers/opcua/driver/client.go Outdated Show resolved Hide resolved
@sailorvii sailorvii force-pushed the temp branch 3 times, most recently from 82ddfbe to c75847b Compare June 1, 2021 02:02
@sailorvii
Add mapper template and refine the make method.
60f159f 
Address fisher's comment.
Remove the useless files.
Reorder the import files.

Signed-off-by: sailorvii <challengingway@hotmail.com>
sailorvii
sailorvii commented Jun 1, 2021
View reviewed changes
Copy link
Collaborator Author

@sailorvii sailorvii left a comment

Choose a reason for hiding this comment

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

Thanks for the review. Refined all issues.

fisherxu
fisherxu reviewed Jun 1, 2021
View reviewed changes
.github/workflows/main.yaml
run: sudo apt-get install -y upx-ucl gcc-aarch64-linux-gnu libc6-dev-arm64-cross gcc-arm-linux-gnueabi libc6-dev-armel-cross

- name: Install golangci-lint
run: curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh| sh -s -- -b $(go env GOPATH)/bin v1.39.0
Copy link
Member

Choose a reason for hiding this comment

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

Why Install golangci-lint when making Multiple docker image build?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because in the building process, it will run all steps including verify, lint, build.

Copy link
Member

Choose a reason for hiding this comment

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

So it's not reasonable to run the lint when make build.

Makefile
@$(curr_dir)/hack/make-rules/$@.sh $(rest_args)

.DEFAULT_GOAL := all
.PHONY: $(make_rules) build
Copy link
Member

Choose a reason for hiding this comment

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

How to run make verify now? It's better to split the verify and build...

Copy link
Member

Choose a reason for hiding this comment

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

We can split them in follow-up PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The lint command includes:
go mod tidy
go mod vendor
gofmt
golangci-lint
If you want to build only, you could run: make mapper build only

Copy link
Member

Choose a reason for hiding this comment

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

Let's split the make build and make verify later.

fisherxu
fisherxu reviewed Jun 1, 2021
View reviewed changes
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.

/lgtm
/approve

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 1, 2021
@kubeedge-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fisherxu

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 Jun 1, 2021
@kubeedge-bot kubeedge-bot merged commit 65a3bbf into kubeedge:main Jun 1, 2021
@gy95 gy95 mentioned this pull request Jun 2, 2021
Closed
@gy95
Copy link
Member

gy95 commented Jun 15, 2021

@sailorvii not support build docker images? we can run make mapper ble to build bluetooth binary file, And how to build bluetooth images?

@sailorvii
Copy link
Collaborator Author

sailorvii commented Jun 17, 2021 •
edited
Loading

@sailorvii not support build docker images? we can run make mapper ble to build bluetooth binary file, And how to build bluetooth images?

make mapper ble package
You could try "make help" to get all actions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fisherxu fisherxu fisherxu left review comments

@kevin-wangzefeng kevin-wangzefeng Awaiting requested review from kevin-wangzefeng

Assignees

@fisherxu fisherxu

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create mapper template
4 participants
@sailorvii @kubeedge-bot @gy95 @fisherxu