search

openyurtio / yurt-app-manager

The workload controller manager from NodePool level in OpenYurt cluster
Apache License 2.0
6 stars 1 forks source link

yurt ingress use helm reconcile #127

Closed huiwq1990 closed 10 months ago

huiwq1990 commented 1 year ago

Signed-off-by: huiwq1990 huiwq1990@163.com

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line: /kind bug /kind documentation /kind enhancement /kind good-first-issue /kind feature /kind question /kind design /sig ai /sig iot /sig network /sig storage /sig storage

What this PR does / why we need it:

Enhance yurtingress, and use fluxcd install nginx-ingress.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

1) As we need third-party packages, so we upgrade golang to 1.17 and k8s packages are upgraded too; 2) The yurtingress v1alpha1 is deprecated, as there is not way to convert from v1alpha1 to v1alpha2, so we could not smoothly upgrade yurt-app-manager; 3) yurtingress is related to nodepool, they are share the same resource name; 4) yurtingress use helm-controller to implement install or upgrade chart;
5) files like pkg/yurtappmanager/controller/yurtingress/fluxcd_helmrelease_controller.go and pkg/yurtappmanager/util/fluxcd/ are copy from helm-controller with minimum change;

Does this PR introduce a user-facing change?

other Note

demo instance:


cat<<EOF | kubectl apply -f -
---
apiVersion: apps.openyurt.io/v1alpha2
kind: YurtIngress
metadata:
  name: hangzhou
spec:
  enabled: true
  interval: 30s
  values:
    controller:
      image:
        registry: "registry.k8s.io" 
      replicaCount: 1

EOF
openyurt-bot commented 1 year ago

@huiwq1990: GitHub didn't allow me to assign the following users: your_reviewer.

Note that only openyurtio members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. For more information please see the contributor guide

In response to [this](https://github.com/openyurtio/yurt-app-manager/pull/127): >Signed-off-by: huiwq1990 > > > > >#### What type of PR is this? >> Uncomment only one ` /kind <>` line, hit enter to put that in a new line, and remove leading whitespace from that line: >> /kind bug >> /kind documentation >/kind enhancement >> /kind good-first-issue >> /kind feature >> /kind question >> /kind design >> /sig ai >> /sig iot >> /sig network >> /sig storage >> /sig storage > > > >#### What this PR does / why we need it: > >Enhance yurtingress, and use fluxcd install nginx-ingress. > >#### Which issue(s) this PR fixes: > >Fixes # > >#### Special notes for your reviewer: > > >1) As we need some package, so we upgrade golang to 1.17; >2) The yurtingress v1alpha1 is deprecated, as there is not way to convert from v1alpha1 to v1alpha2, so we could not smoothly upgrade `yurt-app-manager`; >3) yurtingress is related to nodepool, they are share the same resource name; >4) yurtingress use [helm-controller](https://github.com/fluxcd/helm-controller) to implement install or upgrade chart; >5) files like `pkg/yurtappmanager/controller/yurtingress/fluxcd_helmrelease_controller.go` and `pkg/yurtappmanager/util/fluxcd/` are copy from helm-controller with minimum change; > >#### Does this PR introduce a user-facing change? > >```release-note > >``` > >#### other Note > > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
openyurt-bot commented 1 year ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: huiwq1990 To complete the pull request process, please assign kadisi You can assign the PR to them by writing /assign @kadisi in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/openyurtio/yurt-app-manager/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
codecov[bot] commented 1 year ago

Codecov Report

Merging #127 (7d795ac) into master (1c77dce) will increase coverage by 5.36%. The diff coverage is 11.91%.

@@            Coverage Diff             @@
##           master     #127      +/-   ##
==========================================
+ Coverage   38.27%   43.63%   +5.36%     
==========================================
  Files          42       44       +2     
  Lines        4776     4879     +103     
==========================================
+ Hits         1828     2129     +301     
+ Misses       2704     2456     -248     
- Partials      244      294      +50
Flag Coverage Δ
unittests 43.63% <11.91%> (+5.36%) :arrow_up:

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

Impacted Files Coverage Δ
...oller/yurtingress/fluxcd_helmrelease_controller.go 0.00% <0.00%> (ø)
...manager/controller/yurtingress/nodepool_handler.go 0.00% <0.00%> (ø)
...r/controller/yurtingress/yurtingress_controller.go 0.00% <0.00%> (-17.65%) :arrow_down:
...er/controller/yurtingress/yurtingress_reconcile.go 23.07% <23.07%> (ø)
...ebhook/yurtingress/v1alpha2/yurtingress_webhook.go 45.16% <83.33%> (ø)
...urtappmanager/webhook/nodepool/nodepool_webhook.go 47.16% <0.00%> (+6.74%) :arrow_up:
...anager/controller/uniteddeployment/pool_control.go 60.74% <0.00%> (+43.92%) :arrow_up:
...appmanager/controller/uniteddeployment/revision.go 50.25% <0.00%> (+47.17%) :arrow_up:
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

zzguang commented 1 year ago

Hi, Qinghui I have no many comments about this PR, just some reminders and questions:

  1. Please check whether the codecov/patch issue needs to be fixed.
  2. Please test it make sure it can work correctly in the nodepool environment.
  3. Whether it can be enhanced to support the condition that users only want to enable ingress for some of the nodepools in future?
  4. Did you take the condition into account that users want to enable nginx ingress controller webhook?
  5. Please update the related docs accordingly to avoid any user confusion for YurtIngress usage.
huiwq1990 commented 1 year ago

@zzguang 3) YurtIngress have a enabled attribute, which control whether install ingresscontroller or not; 4) If we want enable webhook, just set values attribute;