Feat :support create service #46

Mcduller · 2023-04-20T05:05:56Z

Description of your changes

In order to support future webhook sources, support for creating services, and some code refactoring

I have:

Read and followed KubeVela's contribution process.
Add related tests.
Run make reviewable to ensure this PR is ready for review.
Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Special notes for your reviewer

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

codecov · 2023-04-20T05:13:00Z

Codecov Report

Patch coverage: 64.00% and project coverage change: -2.49 ⚠️

Comparison is base (2d521b3) 73.19% compared to head (5d0c438) 70.70%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #46      +/-   ##
==========================================
- Coverage   73.19%   70.70%   -2.49%     
==========================================
  Files          13       16       +3     
  Lines        1134     1386     +252     
==========================================
+ Hits          830      980     +150     
- Misses        262      339      +77     
- Partials       42       67      +25

Flag	Coverage Δ
unittests	`70.70% <64.00%> (-2.49%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ollers/triggerservice/triggerservice_controller.go	`54.45% <52.38%> (ø)`
pkg/source/builtin/cronjob/cronjob.go	`64.28% <64.28%> (ø)`
pkg/executor/executor.go	`89.86% <100.00%> (ø)`
pkg/source/builtin/cronjob/config.go	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

FogDong · 2023-04-20T06:59:58Z

controllers/triggerservice/triggerservice_controller.go

+		return err
+	}
+
+	if err := r.createService(ctx, ts, v); err != nil {


Please check createService first, if the value is true, then create the service.

you mean check before invoking the function? or we can check the value in the function ?

config/definition/default.yaml

FogDong · 2023-04-20T07:47:40Z

config/definition/default.yaml

+                  }
+                }]
+              if parameter.service.clusterIp != _|_ {
+                  clusterIp: parameter.service.clusterIp


Why users need to specify the clusterIP?

Why users need to specify the clusterIP?
my fault, i will remove it.

Co-authored-by: Tianxin Dong <[email protected]>

add unit test Co-authored-by: Tianxin Dong <[email protected]> Signed-off-by: Mcduller <[email protected]>

# Conflicts: # config/definition/default.yaml

* Feat: upgrade k8s.io dependency to 0.26 (kubevela#39) * Feat: upgrade k8s.io dependency to 0.26 Signed-off-by: Yin Da <[email protected]> * Fix: golangci-lint action failure Signed-off-by: Yin Da <[email protected]> --------- Signed-off-by: Yin Da <[email protected]> Signed-off-by: Amit Singh <[email protected]> * Feat: add cluster info to action context (kubevela#43) Feat: add cluster info in action context Signed-off-by: yangsoon <[email protected]> Co-authored-by: yangsoon <[email protected]> Signed-off-by: Amit Singh <[email protected]> * Feat: add cronjob source (kubevela#42) * Refactor: allow the use of singleton for one Source, while also allowing non-singleton Sources, making it more extensible Signed-off-by: Charlie Chiang <[email protected]> * Feat: add cronjob source Signed-off-by: Charlie Chiang <[email protected]> * Chore: update cron lib Signed-off-by: Charlie Chiang <[email protected]> * Feat: do not exit if config contains invalid sources Signed-off-by: Charlie Chiang <[email protected]> * Chore: add tests and fix linter Signed-off-by: Charlie Chiang <[email protected]> * Chore: go mod tidy Signed-off-by: Charlie Chiang <[email protected]> * Docs: add cronjob example Signed-off-by: Charlie Chiang <[email protected]> * fix comments Signed-off-by: Charlie Chiang <[email protected]> * Chore: force lint to use the exact version Signed-off-by: Charlie Chiang <[email protected]> * remove unnecessary dep Signed-off-by: Charlie Chiang <[email protected]> * fix comments Signed-off-by: Charlie Chiang <[email protected]> * use codecov token Signed-off-by: Charlie Chiang <[email protected]> * update lint script Signed-off-by: Charlie Chiang <[email protected]> --------- Signed-off-by: Charlie Chiang <[email protected]> Signed-off-by: Amit Singh <[email protected]> * Feat: support leader election for kube-trigger (kubevela#45) Signed-off-by: yangsoon <[email protected]> Co-authored-by: yangsoon <[email protected]> Signed-off-by: Amit Singh <[email protected]> * adds instruction to enable kube-trigger addon Signed-off-by: Amit Singh <[email protected]> * updates sample.yaml path Signed-off-by: Amit Singh <[email protected]> * fix: updates instructions to apply config files in order Signed-off-by: Amit Singh <[email protected]> * fix: adds the missing api group to the first rule Signed-off-by: Amit Singh <[email protected]> * Revert "Feat: support leader election for kube-trigger (kubevela#45)" This reverts commit 4351008. Removing changes unrelated to this branch Signed-off-by: Amit Singh <[email protected]> * Revert "Feat: add cronjob source (kubevela#42)" This reverts commit 46f5975. Removes changes unrelated to the branch Signed-off-by: Amit Singh <[email protected]> * Revert "Feat: add cluster info to action context (kubevela#43)" This reverts commit e214be5. Removes changes unrelated to the branch Signed-off-by: Amit Singh <[email protected]> * Revert "Feat: upgrade k8s.io dependency to 0.26 (kubevela#39)" This reverts commit a22e316. Removes changes unrelated to the branch Signed-off-by: Amit Singh <[email protected]> --------- Signed-off-by: Yin Da <[email protected]> Signed-off-by: Amit Singh <[email protected]> Signed-off-by: yangsoon <[email protected]> Signed-off-by: Charlie Chiang <[email protected]> Co-authored-by: Somefive <[email protected]> Co-authored-by: yangs <[email protected]> Co-authored-by: yangsoon <[email protected]> Co-authored-by: Charlie Chiang <[email protected]>

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

Mcduller · 2023-05-20T08:20:02Z

@charlie0129 @FogDong please review this pr :)

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

Co-authored-by: Tianxin Dong <[email protected]>

add unit test Co-authored-by: Tianxin Dong <[email protected]> Signed-off-by: Mcduller <[email protected]>

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

# Conflicts: # controllers/triggerservice/suite_test.go # controllers/utils/test_util.go

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

# Conflicts: # controllers/utils/test_util.go

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

FogDong · 2023-06-19T10:02:02Z

controllers/triggerservice/triggerservice_controller.go

+		return err
+	}
+
+	if !needCreateService {


If the needCreateService is false, then we'll try to delete the existing service? I think it is a dangerous behavior and we shouldn't clean the service for user automatically.

should we add another flag to clean the service when user want to clean service？

FogDong · 2023-06-19T10:04:50Z

config/manager/role.yaml

- resources:
+- apiGroups:
+  - ""
+  resources:


Is this change caused by our scripts? /cc @charlie0129

Test:support create service

9775e3f

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

Mcduller changed the title ~~Test:support create service~~ Feat :support create service Apr 20, 2023

Mcduller marked this pull request as ready for review April 20, 2023 05:52

Mcduller requested review from FogDong, charlie0129 and Somefive as code owners April 20, 2023 05:52

Mcduller marked this pull request as draft April 20, 2023 05:56

FogDong reviewed Apr 20, 2023

View reviewed changes

config/definition/default.yaml Outdated Show resolved Hide resolved

FogDong reviewed Apr 20, 2023

View reviewed changes

config/definition/default.yaml Outdated Show resolved Hide resolved

FogDong reviewed Apr 20, 2023

View reviewed changes

config/definition/default.yaml Outdated Show resolved Hide resolved

FogDong reviewed Apr 20, 2023

View reviewed changes

Mcduller and others added 11 commits April 20, 2023 16:09

Update config/definition/default.yaml

e015708

Co-authored-by: Tianxin Dong <[email protected]>

Update config/definition/default.yaml

1672387

Co-authored-by: Tianxin Dong <[email protected]>

Update config/definition/default.yaml

1fc1e40

Co-authored-by: Tianxin Dong <[email protected]>

Update config/definition/default.yaml

d94dbc3

add unit test Co-authored-by: Tianxin Dong <[email protected]> Signed-off-by: Mcduller <[email protected]>

Merge remote-tracking branch 'origin/create_svc' into create_svc

5dbf380

# Conflicts: # config/definition/default.yaml

fix unit test

d7f75b1

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

Merge remote-tracking branch 'origin/create_svc' into create_svc

cc9781b

fix unit test

c90ef38

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

Merge remote-tracking branch 'origin/create_svc' into create_svc

6b882f6

fix unit test

40dc64f

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

Mcduller marked this pull request as ready for review May 20, 2023 08:17

Mcduller and others added 4 commits June 18, 2023 10:29

Test:support create service

64956ef

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

Update config/definition/default.yaml

2889aab

Co-authored-by: Tianxin Dong <[email protected]>

Update config/definition/default.yaml

dddc1e5

Co-authored-by: Tianxin Dong <[email protected]>

Update config/definition/default.yaml

25c23fd

add unit test Co-authored-by: Tianxin Dong <[email protected]> Signed-off-by: Mcduller <[email protected]>

Mcduller added 6 commits June 18, 2023 10:33

fix unit test

729aaa3

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

Merge remote-tracking branch 'origin/create_svc' into create_svc

4e33ba3

# Conflicts: # controllers/triggerservice/suite_test.go # controllers/utils/test_util.go

fix unit test

aea9af9

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

Merge remote-tracking branch 'origin/create_svc' into create_svc

919a3cb

# Conflicts: # controllers/utils/test_util.go

add unit test restart pod

e2eeb4d

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

Merge remote-tracking branch 'origin/create_svc' into create_svc

5d0c438

FogDong reviewed Jun 19, 2023

View reviewed changes

config/manager/role.yaml

- resources:

- apiGroups:

- ""

resources:

Copy link

Member

FogDong Jun 19, 2023

Choose a reason for hiding this comment

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

Is this change caused by our scripts? /cc @charlie0129

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat :support create service #46

Feat :support create service #46

Mcduller commented Apr 20, 2023 •

edited

Loading

codecov bot commented Apr 20, 2023 •

edited

Loading

FogDong Apr 20, 2023

Mcduller Apr 20, 2023

FogDong Apr 20, 2023

Mcduller Apr 20, 2023

Mcduller commented May 20, 2023

FogDong Jun 19, 2023

Mcduller Jun 19, 2023

FogDong Jun 19, 2023

Feat :support create service #46

Are you sure you want to change the base?

Feat :support create service #46

Conversation

Mcduller commented Apr 20, 2023 • edited Loading

Description of your changes

How has this code been tested

Special notes for your reviewer

codecov bot commented Apr 20, 2023 • edited Loading

Codecov Report

FogDong Apr 20, 2023

Choose a reason for hiding this comment

Mcduller Apr 20, 2023

Choose a reason for hiding this comment

FogDong Apr 20, 2023

Choose a reason for hiding this comment

Mcduller Apr 20, 2023

Choose a reason for hiding this comment

Mcduller commented May 20, 2023

FogDong Jun 19, 2023

Choose a reason for hiding this comment

Mcduller Jun 19, 2023

Choose a reason for hiding this comment

FogDong Jun 19, 2023

Choose a reason for hiding this comment

Mcduller commented Apr 20, 2023 •

edited

Loading

codecov bot commented Apr 20, 2023 •

edited

Loading