chore(*): best practice for api definition #5

supereagle · 2020-04-22T10:46:24Z

What this PR does / why we need it:

Best practice for api definition for projects using Nirvana framework.

Which issue(s) this PR is related to (optional, link to 3rd issue(s)):

Fixes #

Reference to #

Special notes for your reviewer:

/cc @your-reviewer

Release note:

NONE

supereagle · 2020-04-22T10:53:38Z

Wait for reviews

/hold

lichuan0620

so minor issues with the position of middlewares

the major issue is, we wrote so much that this repo essentially becomes nirvana-demo instead of nirvana-practice

lichuan0620 · 2020-04-22T10:58:28Z

cmd/server/main.go

-	apis.Install(config)
+	config := nirvana.NewDefaultConfig().Configure(
+		nirvana.Port(httpPort),
+		reqlog.Default(),


enabling reqlog at base level might not be a good idea; best adding it at /api so that no log will be printed for /healthz and /metircs

Maybe reqlog can only be applied to base level. If we want to add it to /api level, we need to write a log middleware by ourselves.

Please confirm @iawia002

I'm standing with @lichuan0620, we don't need to log every request such as /healthz and /metrics, it's useless. We recommend logging only helpful request such as /api/*

Have used the log middleware in go-common.

lichuan0620 · 2020-04-22T11:00:23Z

pkg/apis/v1alpha1/descriptors/install.go

+	return definition.Descriptor{
+		Description: "v1alpha1 API",
+		Path:        "/v1alpha1",
+		Consumes:    []string{definition.MIMEJSON},


unless not all APIs are JSON, those should be added only at /api level

Add comments to illustrate that other content types can be added if necessary.

lichuan0620 · 2020-04-22T11:05:58Z

pkg/apis/v1alpha1/handlers/products.go

 	return nil, errors.ErrorNotImplemented.Error()
 }

 func ListProducts(ctx context.Context, options *v1.ListOptions) (*api.ProductsList, error) {
-	return nil, errors.ErrorNotImplemented.Error()


I see that all incomplete functions have been implemented. Doesn't that defeat the purpose of this repo?

nirvana-practice is a public repo meant for helping people practice writing web server with Nirvana. If we finish everything, then they have nothing to practice...

Have removed the implementation.

ddysher · 2020-04-23T10:38:40Z

pkg/apis/v1alpha1/handlers/customers.go

@@ -0,0 +1,50 @@
+package handlers


are we moving all business logic to pkg/apis?

Good question. Prefer in pkg/handlers

lgtm

we did discuss this before, the proposed (slightly modified) structure is

. # ├── Gopkg.toml # ├── Makefile # ├── README.md # ├── apis # 存放 apidocs (swagger json) │ ├── api.v1.json # │ └── api.v2.json # ├── bin # 存放编译后的二进制文件 │ └── nirvana-myproject # ├── build # 存放 Dockerfile │ └── nirvana-myproject # │ └── Dockerfile # ├── cmd # 存放项目的启动命令 │ └── nirvana-myproject # │ └── main.go # ├── nirvana.yaml # ├── pkg # 存放 api 需要用到的结构体与相应的 converters, 按版本区分 │ ├── apis # │ │ └── v1 # │ │ ├── converters # │ │ │ └── converters.go # │ │ └── types.go # │ │ ├── descriptors # │ │ ├── descriptors.go # │ │ └── message.go # 对应 message 业务逻辑的 API 定义 │ ├── filters # 存放 HTTP Request 过滤器 │ │ └── filter.go # │ ├── handler # 存放 API 需要用到的逻辑处理 │ │ └── message.go # │ ├── middlewares # 存放中间件 │ │ └── middlewares.go # │ ├── modifiers # 存放 Definition 修改器 │ │ └── modifiers.go # │ └── version # 项目版本信息目录 │ └── version.go # └── vendor #

a unified project structure definitely makes it easier to read new projects

3.0 might be a good chance to reinforce this idea since many of us will be migrating our repos for COS

Will enforce the unification of projects' structure in v3.0.

Is this not the standard structure generated by nirvana init?

Nope, see #5 (comment). The question is do we need to discuss the structure again?

Strictly speaking, handler/middlewares/modifiers should all be moved into pkg/apis/v1 (e.g. you can't say for sure that modifiers won't touch any v1 data structure).

But this makes the code hard to reason about. I'd vote for current structure and keep it as is. The pkg/apis is only used for user-facing structures and routes.

But this makes the code hard to reason about. I'd vote for current structure and keep it as is. The pkg/apis is only used for user-facing structures and routes.

Agree

handler or handlers?

Do we need to separate handler/middlewares/modifiers into different versions like v1 and v2?

handler

no separation inside handler/middlewares/modifiers

Agreed?

I think we can move to caicloud/nirvana#318 for more discussions instead of here.

supereagle · 2020-04-27T09:05:22Z

Have fixed the comments. @lichuan0620 @iawia002 @ddysher PTAL

supereagle · 2020-04-27T09:05:46Z

/hold cancel

supereagle

⚠️Highlight the key points.

supereagle · 2020-04-27T09:06:56Z

pkg/apis/api.go

+		Description: "APIs",
+		Path:        "/api",
+		Middlewares: []def.Middleware{
+			middleware.Reqlog(log.DefaultLogger()),


Use the log middleware in go-common

go-common is a private repo and this is a public repo

Have copy the log middleware from go-common to this repo, PTAL.

Maybe we can just change go-common to a public repo?

My concern is that once we start advocating this practice, then people will start to use this copied version of log package, instead of the one in go-common, making it difficult to maintain.

Maybe we can just change go-common to a public repo?

I don't like that idea, at least not now. It'll make things more complicated.

Instead, why don't we add some of the more mature features from go-common to Nirvana? One thing we should definitely think about is to implement some built-in middleware in Nirvana. reqlog is obvious. The metrics plugin would also use a middleware implementation (you don't usually want request metrics on the /metrics API).

SGTM, we can move the plugins into nirvana core, or create another dedicated nirvana-plugins repo.

supereagle · 2020-04-27T09:08:38Z

go.mod

+	github.com/caicloud/go-common v0.3.5
+	github.com/caicloud/nirvana v0.2.4


The dependencies require go-common v0.3.5+ and nirvana v0.2.4+.

supereagle · 2020-04-27T09:09:20Z

nirvana.yaml

+- name: v1alpha1
+  description: The v1alpha1 version of this project
+  rules:
+  - prefix: /api/v1alpha1/


You can add more versions if necessary.

supereagle · 2020-04-27T09:10:17Z

pkg/apis/v1alpha1/descriptors/customers.go

+	{
+		Path:        "/customers",
+		Description: "Customer API",
+		Tags:        []string{"customer"},


Add tags to classify APIs.

ddysher · 2020-04-28T03:38:08Z

This seems to be different from nirvana template project,

descriptors should be put under pkg/, instead of pkg/apis
a top-level descriptor.go
maybe some others

Personally, structure nirvana template project makes more sense to me.

/cc @iawia002

iawia002 · 2020-04-28T03:49:49Z

Yes, this is different from nirvana template project cause this project is generated by nirvana init, and the template in nirvana init hasn't synced with the nirvana template project repo.

ddysher · 2020-04-28T03:57:33Z

We need a single source of truth; once this is finalized, we need to converge all these tools/repos.

nirvana init is the single source of truth
template repo is created from nirvana init (with docs pointing to the fact that it's generated from nirvana init)
practice should be forked from template repo

All future changes should go to nirvana init.

An alternative might be to merge nirvana init and template repo.

@iawia002 make the call?

iawia002 · 2020-04-28T04:04:40Z

We need a single source of truth; once this is finalized, we need to converge all these tools/repos.

nirvana init is the single source of truth

I have submitted an issue about this: caicloud/nirvana#304

template repo is created from nirvana init (with docs pointing to the fact that it's generated from nirvana init)
..
An alternative might be to merge nirvana init and template repo.

Yes, I think we can deprecate nirvana-template-project repo, all nirvana projects can be easily generated by nirvana init xxx.

ddysher · 2020-04-28T04:08:37Z

SGTM

Yes, I think we can deprecate nirvana-template-project repo, all nirvana projects can be easily generated by nirvana init xxx.

supereagle · 2020-04-28T06:13:46Z

Yes, I think we can deprecate nirvana-template-project repo, all nirvana projects can be easily generated by nirvana init xxx.

SGTM

lichuan0620 · 2020-04-28T08:00:21Z

/lgtm

caicloud-bot · 2020-04-28T08:00:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lichuan0620

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:

~~OWNERS~~ [lichuan0620]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

supereagle added 2 commits April 22, 2020 18:44

chore(vendor): switch to go mod and add vendor

dd5ccc2

chore(*): best practice for api definition

40e2c1c

caicloud-bot added release-note-none Denotes a PR that doesn't merit a release note. caicloud-cla: yes Indicates the PR's author has not signed the Caicloud CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 22, 2020

caicloud-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 22, 2020

lichuan0620 requested changes Apr 22, 2020

View reviewed changes

ddysher reviewed Apr 23, 2020

View reviewed changes

fix comments

4ec02a0

caicloud-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 27, 2020

supereagle commented Apr 27, 2020

View reviewed changes

supereagle added 2 commits April 28, 2020 00:20

update vendor

ebf002b

implement log middleware

a263e9b

supereagle force-pushed the api-docs branch from 4b98e1d to a263e9b Compare April 27, 2020 16:21

caicloud-bot requested a review from iawia002 April 28, 2020 03:38

caicloud-bot assigned lichuan0620 Apr 28, 2020

caicloud-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 28, 2020

caicloud-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 28, 2020

caicloud-bot merged commit 7a6f3e7 into caicloud:master Apr 28, 2020

supereagle mentioned this pull request Apr 30, 2020

Recommended structures for Nirvana projects caicloud/nirvana#318

Open

iawia002 mentioned this pull request May 17, 2020

refactor(*): update template content in the init command caicloud/nirvana#323

Merged

		github.com/caicloud/go-common v0.3.5
		github.com/caicloud/nirvana v0.2.4

chore(*): best practice for api definition #5

chore(*): best practice for api definition #5

Conversation

supereagle commented Apr 22, 2020

supereagle commented Apr 22, 2020

lichuan0620 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supereagle Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supereagle Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddysher Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddysher Apr 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supereagle Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

supereagle commented Apr 27, 2020

supereagle commented Apr 27, 2020

supereagle left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supereagle Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddysher commented Apr 28, 2020

iawia002 commented Apr 28, 2020

ddysher commented Apr 28, 2020

iawia002 commented Apr 28, 2020

ddysher commented Apr 28, 2020

supereagle commented Apr 28, 2020

lichuan0620 commented Apr 28, 2020

caicloud-bot commented Apr 28, 2020

supereagle Apr 23, 2020 •

edited

Loading

supereagle Apr 27, 2020 •

edited

Loading

ddysher Apr 23, 2020 •

edited

Loading

ddysher Apr 24, 2020 •

edited

Loading

supereagle Apr 30, 2020 •

edited

Loading

supereagle left a comment •

edited

Loading

supereagle Apr 27, 2020 •

edited

Loading