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

refactor(suggestion): Use interface #502

Merged
merged 8 commits into from
May 14, 2019
Merged

refactor(suggestion): Use interface #502

merged 8 commits into from
May 14, 2019

Conversation

gaocegege
Copy link
Member

@gaocegege gaocegege commented May 13, 2019 •
edited
Loading

There are some benefits to define an interface instead of using util.GetSuggestions function:

  • Easy to test and maintain. We can write unit test cases for the interface.
  • Easy to integrate. We can mock the suggestion when testing the experiment controller, or use a fake controller to run the experiment without affecting the real logic.

This PR:

  • added a CLI argument to control if we use fake implementation in the controller. If not, now we will always return an error becuase we do not implement suggestion logic now.
  -fake-experiment-suggestion
    	Use the fake suggestion in experiment controller
  • added a interface about suggestion.

Signed-off-by: Ce Gao gaoce@caicloud.io

This change is Reviewable

@gaocegege
Copy link
Member Author

/cc @johnugeorge @richardsliu @YujiOshima

PTAL

@johnugeorge
Copy link
Member

/lgtm

@gaocegege gaocegege mentioned this pull request May 13, 2019
Closed
gaocegege
gaocegege commented May 13, 2019
View reviewed changes
Copy link
Member Author

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot k8s-ci-robot removed the lgtm label May 13, 2019
@gaocegege
Copy link
Member Author

@hougangliu PTAL

hougangliu
hougangliu reviewed May 13, 2019
View reviewed changes
cmd/katib-controller/v1alpha2/main.go Outdated Show resolved Hide resolved
pkg/controller/v1alpha2/consts/const.go Outdated Show resolved Hide resolved
pkg/controller/v1alpha2/experiment/experiment_controller.go Outdated Show resolved Hide resolved
pkg/controller/v1alpha2/experiment/experiment_controller.go Outdated Show resolved Hide resolved
@hougangliu
Copy link
Member

hougangliu commented May 13, 2019 •
edited
Loading

@hougangliu PTAL

@gaocegege Thanks. some mini comment

@gaocegege
Copy link
Member Author

@hougangliu PTAL
@gaocegege Thanks. some mini comment

Addressed, Thanks for your review. PTAL

@hougangliu
Copy link
Member

/lgtm
thanks

@k8s-ci-robot k8s-ci-robot removed the lgtm label May 13, 2019
@hougangliu
Copy link
Member

/lgtm

@YujiOshima
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gaocegege, YujiOshima

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 [YujiOshima,gaocegege]

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

@k8s-ci-robot k8s-ci-robot removed the lgtm label May 13, 2019
@gaocegege
Copy link
Member Author

@hougangliu PTAL again, fixed a bug.

@gaocegege
Copy link
Member Author

/retest

2 similar comments
@gaocegege
Copy link
Member Author

/retest

@gaocegege
Copy link
Member Author

/retest

johnugeorge
johnugeorge reviewed May 13, 2019
View reviewed changes
pkg/controller/v1alpha2/experiment/experiment_controller.go
Client: mgr.GetClient(),
scheme: mgr.GetScheme(),
}
imp := viper.GetString(consts.ConfigExperimentSuggestionName)
Copy link
Member

Choose a reason for hiding this comment

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

Just to check: Is there a better way to pass flags to the manager than using viper?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea. Kubebuilder does not support a built-in way, thus I use viper here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does it work for you?

@gaocegege
Copy link
Member Author

/retest

2 similar comments
@gaocegege
Copy link
Member Author

/retest

@gaocegege
Copy link
Member Author

/retest

@gaocegege
refactor(suggestion): Use interface
2389679 
Signed-off-by: Ce Gao <gaoce@caicloud.io>
@gaocegege
feat(cmd): Add flags to control the fake logic
00e9d37 
Signed-off-by: Ce Gao <gaoce@caicloud.io>
@gaocegege
gopkg: Update
aa8d6bc 
Signed-off-by: Ce Gao <gaoce@caicloud.io>
@gaocegege
fix: Revert package names
c248c03 
Signed-off-by: Ce Gao <gaoce@caicloud.io>
@gaocegege
fix: Use string
8dccf3f 
Signed-off-by: Ce Gao <gaoce@caicloud.io>
@gaocegege
fix: Address comments
5117097 
Signed-off-by: Ce Gao <gaoce@caicloud.io>
@gaocegege
fix: Address comment
82a0d84 
Signed-off-by: Ce Gao <gaoce@caicloud.io>
@gaocegege
fix: Fix bug
dc31fa8 
Signed-off-by: Ce Gao <gaoce@caicloud.io>
@gaocegege
Copy link
Member Author

/retest

4 similar comments
@gaocegege
Copy link
Member Author

/retest

@gaocegege
Copy link
Member Author

/retest

@gaocegege
Copy link
Member Author

/retest

@gaocegege
Copy link
Member Author

/retest

@johnugeorge
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot merged commit 5478db4 into kubeflow:master May 14, 2019
@gaocegege gaocegege deleted the re branch May 15, 2019 01:14
@andreyvelich andreyvelich mentioned this pull request Jun 24, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnugeorge johnugeorge johnugeorge left review comments

@YujiOshima YujiOshima YujiOshima left review comments

@hougangliu hougangliu hougangliu left review comments

@Akado2009 Akado2009 Awaiting requested review from Akado2009

@jinan-zhou jinan-zhou Awaiting requested review from jinan-zhou

@richardsliu richardsliu Awaiting requested review from richardsliu

Assignees

@johnugeorge johnugeorge

@hougangliu hougangliu

Labels
approved lgtm size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gaocegege @johnugeorge @hougangliu @YujiOshima @k8s-ci-robot