Modify API and Generate Addon API clientset/lister/informers #111

ccfishk · 2022-02-20T20:47:22Z

Fixes:
#97
https://github.intuit.com/kubernetes/arktika/issues/3541

Use code-generator generate API(s) clientset/informer/listers and put into pkg/client directory.
Add code-generator into Makefile
move api/v1alpha1 to apis/addon/v1alpha1, and add "comment marker" for code generator

Signed-off-by: jhu02 <[email protected]>

pkg/apis/addon/v1alpha1/addon_types.go

figo

Thank you for working on this refactoring effort, Refactor is good, but would like to share a few guideline to help both reviewers and PR owners.

In the PR description, document what problem the PR trying to solve (break the large PR into smaller ones, and make it incremental as much as possible), "refactoring" is the approach, but it is not a problem statement,
To make sure reviewer can review the PR thoroughly, add the dependencies and steps that generate the PR into a script (in this case, kubebuilder version etc), include the script in the PR, this will make sure others can also generate the PR using the script, this apply to PR when there is generated code.
(optional) walk through the PR with reviewers, and share the plan for the following PRs.

Thanks again.

ccfishk · 2022-02-20T22:33:49Z

Thank you for working on this refactoring effort, Refactor is good, but would like to share a few guideline to help both reviewers and PR owners.

In the PR description, document what problem the PR trying to solve (break the large PR into smaller ones, and make it incremental as much as possible), "refactoring" is the approach, but it is not a problem statement,

To make sure reviewer can review the PR thoroughly, add the dependencies and steps that generate the PR into a script (in this case, kubebuilder version etc), include the script in the PR, this will make sure others can also generate the PR using the script, this apply to PR when there is generated code.

(optional) walk through the PR with reviewers, and share the plan for the following PRs.

Thanks again.
This comments justifying the strategy
#99 (comment)

figo · 2022-02-20T22:42:51Z

Thank you for working on this refactoring effort, Refactor is good, but would like to share a few guideline to help both reviewers and PR owners.

In the PR description, document what problem the PR trying to solve (break the large PR into smaller ones, and make it incremental as much as possible), "refactoring" is the approach, but it is not a problem statement,

To make sure reviewer can review the PR thoroughly, add the dependencies and steps that generate the PR into a script (in this case, kubebuilder version etc), include the script in the PR, this will make sure others can also generate the PR using the script, this apply to PR when there is generated code.

(optional) walk through the PR with reviewers, and share the plan for the following PRs.

Thanks again.
This comments justifying the strategy
#99 (comment)

Yes, glad those 3 action items make sense to you, could you start to address them?

ccfishk · 2022-02-22T19:21:15Z

Thank you for working on this refactoring effort, Refactor is good, but would like to share a few guideline to help both reviewers and PR owners.

In the PR description, document what problem the PR trying to solve (break the large PR into smaller ones, and make it incremental as much as possible), "refactoring" is the approach, but it is not a problem statement,

To make sure reviewer can review the PR thoroughly, add the dependencies and steps that generate the PR into a script (in this case, kubebuilder version etc), include the script in the PR, this will make sure others can also generate the PR using the script, this apply to PR when there is generated code.

(optional) walk through the PR with reviewers, and share the plan for the following PRs.

Thanks again.
This comments justifying the strategy
#99 (comment)

Yes, glad those 3 action items make sense to you, could you start to address them?

Documented the problem details in following ticket tracking all the solutions that are applied.
#99 (comment)

Signed-off-by: jhu02 <[email protected]>

…ordingly Signed-off-by: jhu02 <[email protected]>

Signed-off-by: jhu02 <[email protected]>

Makefile

…ement Signed-off-by: jhu02 <[email protected]>

Signed-off-by: jhu02 <[email protected]>

…set reference Signed-off-by: jhu02 <[email protected]>

Signed-off-by: jhu02 <[email protected]>

Dockerfile

Makefile

Signed-off-by: jhu02 <[email protected]>

e2e/addon_types_test.go

e2e/suite_test.go

Makefile

…y with kuberbuilder comment marker Signed-off-by: jhu02 <[email protected]>

Signed-off-by: jhu02 <[email protected]>

kevdowney · 2022-02-25T21:09:48Z

Makefile

-test: generate fmt vet manifests
-	go test ./api/... ./controllers/... ./pkg/... ./cmd/... -coverprofile cover.out
+.PHONY: test
+test:


Needs the other targets also.

Suggested change

test:

test: generate fmt vet manifests

kevdowney · 2022-02-25T21:18:56Z

Makefile

-	$(CONTROLLER_GEN) object:headerFile=./hack/boilerplate.go.txt paths=./api/...
+generate: controller-gen code-generator
+	$(CONTROLLER_GEN) object:headerFile=./hack/boilerplate.go.txt paths=./apis/addon/...
+	bash $(GOPATH)/src/k8s.io/[email protected]/generate-groups.sh \


Let's capture this code-generator call as env. var, $(CODE_GEN)

kevdowney · 2022-02-25T21:19:32Z

Makefile

+	cd $$CODE_GENERATOR_DIR ;\
+	go get github.com/kubernetes/[email protected] ;\
+	}
+endif


Suggested change

endif

else

CODE_GEN="bash $(GOPATH)/src/k8s.io/[email protected]/generate-groups.sh"

endif

ccfishk · 2022-02-26T00:43:58Z

per conversation, this PR is moved to #116.

ccfishk · 2022-02-26T01:25:29Z

close as it is.

Generate Addon API client and mve them into pkg directory

2a5095b

Signed-off-by: jhu02 <[email protected]>

ccfishk requested a review from a team as a code owner February 20, 2022 20:47

ccfishk requested a review from kevdowney February 20, 2022 20:48

Modify Makefile accordingly

e1d6816

Signed-off-by: jhu02 <[email protected]>

ccfishk mentioned this pull request Feb 20, 2022

remove duplicate workflow installation and deletion #99

Closed

ccfishk requested a review from tekenstam February 20, 2022 20:53

kevdowney reviewed Feb 20, 2022

View reviewed changes

pkg/apis/addon/v1alpha1/addon_types.go Outdated Show resolved Hide resolved

figo requested changes Feb 20, 2022

View reviewed changes

Change: add code-generator into Makefile

6c81fde

Signed-off-by: jhu02 <[email protected]>

ccfishk force-pushed the apiclient branch from cf503ce to 6c81fde Compare February 24, 2022 22:34

jhu02 and others added 7 commits February 24, 2022 15:29

Change: update Dockerfile

5f8fbf1

Signed-off-by: jhu02 <[email protected]>

Fix: update makefile with code-generator

d05c872

Signed-off-by: jhu02 <[email protected]>

Merge branch 'master' into apiclient

2e37ef3

Revert apis back to prevous directory; change dockerfile/Makefile acc…

6923186

…ordingly Signed-off-by: jhu02 <[email protected]>

Change: revert Makefile also

5392c12

Signed-off-by: jhu02 <[email protected]>

revert more changes

6bd489c

Signed-off-by: jhu02 <[email protected]>

Fix: revert broken

b9f9931

Signed-off-by: jhu02 <[email protected]>

kevdowney reviewed Feb 25, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

jhu02 added 6 commits February 24, 2022 18:07

Change: split tests into specific directory for better problem resolv…

2729a24

…ement Signed-off-by: jhu02 <[email protected]>

Fix typo

b72bc36

Signed-off-by: jhu02 <[email protected]>

Change: move api_types_test.go to controlers directory for API client…

3b8a7e4

…set reference Signed-off-by: jhu02 <[email protected]>

Change: avoid import loop if api_types.go is kept in API dir

6b96efd

Signed-off-by: jhu02 <[email protected]>

Fix: api tests

e0dbd6e

Signed-off-by: jhu02 <[email protected]>

Fix api tests

07252dc

Signed-off-by: jhu02 <[email protected]>

ccfishk requested review from kevdowney and figo February 25, 2022 07:16

Fix api tests

dde7e92

Signed-off-by: jhu02 <[email protected]>

jhu02 added 2 commits February 25, 2022 08:33

Fix: api tests setup

55ee687

Signed-off-by: jhu02 <[email protected]>

Fix: cleanup

15fa61c

Signed-off-by: jhu02 <[email protected]>

ccfishk changed the title ~~Generate Addon API clientset/lister/informers~~ Modify API and Generate Addon API clientset/lister/informers Feb 25, 2022

kevdowney reviewed Feb 25, 2022

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

kevdowney reviewed Feb 25, 2022

View reviewed changes

Makefile Show resolved Hide resolved

kevdowney reviewed Feb 25, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Fix: cleanup

28000e0

Signed-off-by: jhu02 <[email protected]>

kevdowney reviewed Feb 25, 2022

View reviewed changes

e2e/addon_types_test.go Outdated Show resolved Hide resolved

kevdowney reviewed Feb 25, 2022

View reviewed changes

e2e/suite_test.go Outdated Show resolved Hide resolved

kevdowney reviewed Feb 25, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

jhu02 added 3 commits February 25, 2022 10:51

Changes: revert api_types_test.go and suit_test.go; test compitabilit…

0ce90df

…y with kuberbuilder comment marker Signed-off-by: jhu02 <[email protected]>

Fix: move code-generator under generate

446529d

Signed-off-by: jhu02 <[email protected]>

Changes: add code-generator into the right place

5f4789d

Signed-off-by: jhu02 <[email protected]>

kevdowney reviewed Feb 25, 2022

View reviewed changes

ccfishk removed the request for review from figo February 26, 2022 00:39

ccfishk closed this Feb 26, 2022

kevdowney deleted the apiclient branch May 1, 2023 16:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify API and Generate Addon API clientset/lister/informers #111

Modify API and Generate Addon API clientset/lister/informers #111

ccfishk commented Feb 20, 2022 •

edited

Loading

figo left a comment

ccfishk commented Feb 20, 2022

figo commented Feb 20, 2022

ccfishk commented Feb 22, 2022

kevdowney Feb 25, 2022

kevdowney Feb 25, 2022

kevdowney Feb 25, 2022

ccfishk commented Feb 26, 2022

ccfishk commented Feb 26, 2022

-endif
+else
+CODE_GEN="bash $(GOPATH)/src/k8s.io/[email protected]/generate-groups.sh"
+endif

Modify API and Generate Addon API clientset/lister/informers #111

Modify API and Generate Addon API clientset/lister/informers #111

Conversation

ccfishk commented Feb 20, 2022 • edited Loading

figo left a comment

Choose a reason for hiding this comment

ccfishk commented Feb 20, 2022

figo commented Feb 20, 2022

ccfishk commented Feb 22, 2022

kevdowney Feb 25, 2022

Choose a reason for hiding this comment

kevdowney Feb 25, 2022

Choose a reason for hiding this comment

kevdowney Feb 25, 2022

Choose a reason for hiding this comment

ccfishk commented Feb 26, 2022

ccfishk commented Feb 26, 2022

ccfishk commented Feb 20, 2022 •

edited

Loading