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

crd naming additional modifications #118

Closed YTGhost closed 2 years ago

YTGhost commented 2 years ago

Signed-off-by: HIHIA 283304489@qq.com

What type of PR is this?

/kind feature

What this PR does / why we need it:

After https://github.com/openyurtio/yurt-app-manager/pull/117, I found that some of the yaml files also need to be updated

Which issue(s) this PR fixes:

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

Special notes for your reviewer:

Does this PR introduce a user-facing change?

other Note

openyurt-bot commented 2 years ago

@YTGhost: 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/118): >Signed-off-by: HIHIA <283304489@qq.com> > > > > >#### What type of PR is this? >/kind feature > > > >#### What this PR does / why we need it: >After https://github.com/openyurtio/yurt-app-manager/pull/117, I found that some of the yaml files also need to be updated > >#### Which issue(s) this PR fixes: > >Fixes https://github.com/openyurtio/openyurt/issues/852 > >#### Special notes for your reviewer: > > > >#### 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.
codecov[bot] commented 2 years ago

Codecov Report

Merging #118 (6ea421c) into master (dfea469) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #118   +/-   ##
=======================================
  Coverage   37.37%   37.37%           
=======================================
  Files          42       42           
  Lines        4776     4776           
=======================================
  Hits         1785     1785           
  Misses       2752     2752           
  Partials      239      239
Flag Coverage Δ
unittests 37.37% <ø> (ø)

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

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

rambohe-ch commented 2 years ago

@zzguang PTAL

Congrool commented 2 years ago

We seems to use make generate-deploy-yaml to generate yamls for all crds. Do you modify it manually or use the make command? @YTGhost

By the way, do we still use all-in-one.yaml to install yurt-app-manager? PTAL @huiwq1990

YTGhost commented 2 years ago

We seems to use make generate-deploy-yaml to generate yamls for all crds. Do you modify it manually or use the make command? @YTGhost

By the way, do we still use all-in-one.yaml to install yurt-app-manager? PTAL @huiwq1990

Yep, I use make generate-deploy-yaml to generate yamls for all crds, and then use the content to modify these two files.

luc99hen commented 2 years ago

We seems to use make generate-deploy-yaml to generate yamls for all crds. Do you modify it manually or use the make command? @YTGhost

By the way, do we still use all-in-one.yaml to install yurt-app-manager? PTAL @huiwq1990

We recommend to use helm instead of yaml in the latest documentation.

Congrool commented 2 years ago

@luc99hen Thanks.

Since

We recommend to use helm instead of yaml in the latest documentation.

should we continue to maintain all-in-one.yaml in the following version?

luc99hen commented 2 years ago

Since

We recommend to use helm instead of yaml in the latest documentation.

should we continue to maintain all-in-one.yaml in the following version?

I think they are no longer needed. What do you think of it? @rambohe-ch

zzguang commented 2 years ago

Since helm chart will replace the old yaml file style, it may not need to maintain the yaml file any more, please double check it. Besides, for YurtIngress, the related docs need to be updated accordingly:

  1. The proposal doc: https://github.com/openyurtio/openyurt/blob/master/docs/proposals/20210628-nodepool-ingress-support.md
  2. The user tutorial doc: https://github.com/openyurtio/openyurt.io/blob/master/versioned_docs/version-v1.1/user-manuals/network/edge-ingress.md https://github.com/openyurtio/openyurt.io/blob/master/i18n/zh/docusaurus-plugin-content-docs/version-v1.1/user-manuals/network/edge-ingress.md For the related crd naming optimization PRs are assumed to be merged into version v1.1, so I set the version to v1.1 as above.
rambohe-ch commented 2 years ago

We seems to use make generate-deploy-yaml to generate yamls for all crds. Do you modify it manually or use the make command? @YTGhost

By the way, do we still use all-in-one.yaml to install yurt-app-manager? PTAL @huiwq1990

@Congrool agree +1, it's not need to maintain all-in-one.yaml file for each repos, and if end user want to get yaml files, they will be suggested to get yaml files from helm charts.

rambohe-ch commented 2 years ago

@YTGhost I think you can post another pr for removing all-in-one.yaml file.

YTGhost commented 2 years ago

@YTGhost I think you can post another pr for removing all-in-one.yaml file.

ok

YTGhost commented 2 years ago

Since helm chart will replace the old yaml file style, it may not need to maintain the yaml file any more, please double check it. Besides, for YurtIngress, the related docs need to be updated accordingly:

  1. The proposal doc: https://github.com/openyurtio/openyurt/blob/master/docs/proposals/20210628-nodepool-ingress-support.md
  2. The user tutorial doc: https://github.com/openyurtio/openyurt.io/blob/master/versioned_docs/version-v1.1/user-manuals/network/edge-ingress.md https://github.com/openyurtio/openyurt.io/blob/master/i18n/zh/docusaurus-plugin-content-docs/version-v1.1/user-manuals/network/edge-ingress.md For the related crd naming optimization PRs are assumed to be merged into version v1.1, so I set the version to v1.1 as above.

ok，Thanks

YTGhost commented 2 years ago

@zzguang PTAL, now I just modify yaml of chart, I will remove all-in-one.yaml file in another pr.

rambohe-ch commented 2 years ago

@zzguang PTAL

zzguang commented 2 years ago

/lgtm

zzguang commented 2 years ago

/approve

openyurt-bot commented 2 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: YTGhost, zzguang

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)~~ [zzguang] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment