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

Upgrade MeshController to use TrafficController #79

Merged
merged 16 commits into from Jun 29, 2021

Conversation

xxx7xxxx
Copy link
Contributor

No description provided.

@benja-wu benja-wu added the enhancement New feature or request label Jun 28, 2021
@benja-wu benja-wu added this to In progress in Easegress Project via automation Jun 28, 2021
@benja-wu benja-wu added this to the v1.0.1 milestone Jun 28, 2021
Easegress Project automation moved this from In progress to Reviewer approved Jun 28, 2021
Copy link
Contributor

@benja-wu benja-wu left a comment

Choose a reason for hiding this comment

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

Mesh related logic had been tested in KM env, LGTM

Comment on lines +92 to +93
mc.superSpec, mc.spec, mc.super = superSpec, superSpec.ObjectSpec().(*spec.Admin), super
mc.reload()
Copy link
Collaborator

Choose a reason for hiding this comment

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

mc.api is not set as in function Init.

Copy link
Contributor

Choose a reason for hiding this comment

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

@xxx7xxxx , Is that necessary to introduce the api into MeshController? Since master and worker have handled the API registration inside. And I can't find any use of m.api in MeshController.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's expected. Otherwise it will cause repeating registering apis.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if so, in Init, can we change

mc.api = api.New(superSpec, super)

to

api.New(superSpec, super)

and remove mc.api?

it is weird to only initialize the variable in Init.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see the difference. IMO, the standalone api.New is weirder to me. I updated it to api.Register

pkg/object/meshcontroller/worker/worker.go Outdated Show resolved Hide resolved
pkg/object/meshcontroller/worker/worker.go Outdated Show resolved Hide resolved
@benja-wu benja-wu merged commit f6786f7 into easegress-io:main Jun 29, 2021
Easegress Project automation moved this from Reviewer approved to Done Jun 29, 2021
@xxx7xxxx xxx7xxxx deleted the meshcontroller branch June 30, 2021 02:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants