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

feat: rename UnitedDeployment to YurtAppSet #62

Closed xavier-hou closed 2 years ago

xavier-hou commented 2 years ago

Signed-off-by: hxcGit houxc_mail@163.com

What type of PR is this?

/kind good-first-issue

What this PR does / why we need it:

rename UnitedDeployment to YurtAppSet

Which issue(s) this PR fixes:

Fixes https://github.com/openyurtio/openyurt/issues/735

Special notes for your reviewer:

If there is anything you think is unreasonable, please let me know. I will do my best to fix it.

Does this PR introduce a user-facing change?

other Note

I have do some test after changing the name.

  1. make test

    • All test cases work fine except for one test in pkg/yurtappmanager/util/refmanager/refmanager_test.go. But this does not seem to be caused by the name change.

  2. deploy in k8s cluster according to Yurt-app-manager Tutorial

    image
By the way, I would like to know how to test if the webhook is working correctly. And what other tests do I need to take?
openyurt-bot commented 2 years ago

@hxcGit: 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/62): >Signed-off-by: hxcGit > > > > >#### What type of PR is this? >/kind good-first-issue > > >#### What this PR does / why we need it: >rename UnitedDeployment to YurtAppSet > >#### Which issue(s) this PR fixes: > >Fixes https://github.com/openyurtio/openyurt/issues/735 > >#### Special notes for your reviewer: > >If there is anything you think is unreasonable, please let me know. I will do my best to fix it. > >#### Does this PR introduce a user-facing change? > >```release-note > >``` > >#### other Note > > >I have do some test after changing the name. >1. `make test` >- All test cases work fine except for one test in `pkg/yurtappmanager/util/refmanager/refmanager_test.go`. But this seems to have no effect on the name change. > >2. deploy in k8s cluster according to [Yurt-app-manager Tutorial](https://github.com/openyurtio/yurt-app-manager/blob/master/docs/yurt-app-manager-tutorial.md) >image > >##### By the way, I would like to know how to test if the webhook is working correctly. And what other tests do I need to take? 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 2 years ago

Welcome @hxcGit! It looks like this is your first PR to openyurtio/yurt-app-manager 🎉

rambohe-ch commented 2 years ago

/assign @zzguang @kadisi

openyurt-bot commented 2 years ago

@rambohe-ch: GitHub didn't allow me to assign the following users: zzguang.

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/62#issuecomment-1169948376): >/assign @zzguang @kadisi 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 2 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hxcGit, rambohe-ch

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](https://github.com/openyurtio/yurt-app-manager/blob/master/OWNERS)~~ [rambohe-ch] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
huiwq1990 commented 2 years ago

@rambohe-ch we should directly delete uniteddeployment as user already use it. For example, our yurt-edgex-manager use it deploy edgex. https://github.com/openyurtio/yurt-edgex-manager/blob/b0c2315fd148d3d4a226690b0a149b42314cc13f/controllers/edgex_controller.go#L354

huiwq1990 commented 2 years ago

It's better import deprecation warnings follow the document https://kubernetes.io/blog/2020/09/03/warnings/#deprecation-warnings.

rambohe-ch commented 2 years ago

@rambohe-ch we should directly delete uniteddeployment as user already use it. For example, our yurt-edgex-manager use it deploy edgex. https://github.com/openyurtio/yurt-edgex-manager/blob/b0c2315fd148d3d4a226690b0a149b42314cc13f/controllers/edgex_controller.go#L354

@hxcGit would you be able to post a pull request to fix this potential problem?

xavier-hou commented 2 years ago

@rambohe-ch we should directly delete uniteddeployment as user already use it. For example, our yurt-edgex-manager use it deploy edgex. https://github.com/openyurtio/yurt-edgex-manager/blob/b0c2315fd148d3d4a226690b0a149b42314cc13f/controllers/edgex_controller.go#L354

@hxcGit would you be able to post a pull request to fix this potential problem?

@rambohe-ch Sure, I'll take a look. @huiwq1990 Many thanks to your suggestion!