search

karmada-io / karmada

Open, Multi-Cloud, Multi-Cluster Kubernetes Orchestration
https://karmada.io
Apache License 2.0
4.12k stars 806 forks source link

clear repeated cmd #869

Closed 2hangchen closed 2 years ago

2hangchen commented 2 years ago

What type of PR is this?

What this PR does / why we need it: /kind cleanup

Special notes for your reviewer: This command has been executed before (line:118)

lfbear commented 2 years ago

I think it seems not suitable for this cleanup. If we consider it from repeated execution, after the removing, there's only one line in the function, further considered, shall we cancel the installCRDs function.

lfbear commented 2 years ago

On another angle, we usually package code to a function to achieve a specific outcome. I suppose the original of installCRDs maybe wanna package the process of all the resources. If so, we should think about all the dependencies, including the namespace, RBAC, and so on.

2hangchen commented 2 years ago

On another angle, we usually package code to a function to achieve a specific outcome. I suppose the original of installCRDs maybe wanna package the process of all the resources. If so, we should think about all the dependencies, including the namespace, RBAC, and so on.

I agree with you. By the way ,is there any planning for scripts for deployed offine?

lfbear commented 2 years ago

By the way ,is there any planning for scripts for deployed offine?

I am not very clear about deployment offline, as far as know, the pulling image needs internet, but you can save the images to local and change the image URL in YAML files.

2hangchen commented 2 years ago

By the way ,is there any planning for scripts for deployed offine?

I am not very clear about deployment offline, as far as know, the pulling image needs internet, but you can save the images to local and change the image URL in YAML files.

ok，thx

RainbowMango commented 2 years ago

Is there anything we need to address in this PR?

lfbear commented 2 years ago

I suggest we may cancel the installCRDs function first. Use directly kubectl kustomize "${REPO_ROOT}/charts/_crds" | kubectl apply -f - in the code. If necessary later, we can package all the Karmada resources code to one function. How about it @2hangchen

2hangchen commented 2 years ago

I suggest we may cancel the installCRDs function first. Use directly kubectl kustomize "${REPO_ROOT}/charts/_crds" | kubectl apply -f - in the code. If necessary later, we can package all the Karmada resources code to one function. How about it @2hangchen

I think it is fine.

RainbowMango commented 2 years ago

/assign @lfbear

RainbowMango commented 2 years ago

Don't know if this pr is blocked by failing test, just re-triggered.

lfbear commented 2 years ago

I suggest we may cancel the installCRDs function first. Use directly kubectl kustomize "${REPO_ROOT}/charts/_crds" | kubectl apply -f - in the code. If necessary later, we can package all the Karmada resources code to one function. How about it @2hangchen

I think it is fine.

@2hangchen Do you have time to finish this recently? :)

RainbowMango commented 2 years ago

Kind ping @2hangchen @lfbear

lfbear commented 2 years ago

/lgtm

lfbear commented 2 years ago

/remove-lgtm

lfbear commented 2 years ago

@2hangchen Could you merge the master branch and solve the conflict.

2hangchen commented 2 years ago

@2hangchen Could you merge the master branch and solve the conflict.

I'm sorry. I just saw it. I'll deal with it now.

2hangchen commented 2 years ago

/retest

karmada-bot commented 2 years ago

@2hangchen: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to [this](https://github.com/karmada-io/karmada/pull/869#issuecomment-981303417): >/retest 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.
lfbear commented 2 years ago

/ok-to-test

RainbowMango commented 2 years ago 
[BeforeSuite] BeforeSuite 
/home/runner/work/karmada/karmada/test/e2e/suite_test.go:81

  Unexpected error:
      <*errors.errorString | 0xc000585e50>: {
          s: "cluster member1 not ready",
      }
      cluster member1 not ready
  occurred

  /home/runner/work/karmada/karmada/test/e2e/framework/cluster.go:60

Manually re-triggered.

lfbear commented 2 years ago

It seems not the problem with the e2e test. I checked the code again and found that namespace creating can not remove in the function installCRDs because it will create a namespace in the karmada-apiserver cluster and another (line:129 in hack/deploy-karmada.sh) will create a namespace in the karmada-host cluster.

lfbear commented 2 years ago

/remove-approve

karmada-bot commented 2 years ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: To complete the pull request process, please ask for approval from lfbear after the PR has been reviewed.

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

Needs approval from an approver in each of these files: - **[hack/OWNERS](https://github.com/karmada-io/karmada/blob/master/hack/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
2hangchen commented 2 years ago

It seems not the problem with the e2e test. I checked the code again and found that namespace creating can not remove in the function installCRDs because it will create a namespace in the karmada-apiserver cluster and another (line:129 in hack/deploy-karmada.sh) will create a namespace in the karmada-host cluster.

@lfbear I get it. I got it wrong