Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

optimize: using namespace from command line when deployment with helm charts #6029

Conversation

baiyangtx
Copy link
Contributor

  • I have registered the PR changes.

Ⅰ. Describe what this PR did

  1. Currently, the namespace set in Values.yaml only takes effect for deployment-type resources, while service and pod resources ignore this value.
  2. It is customary to specify the namespace using the -n parameter in the helm install command, but currently, deployment ignores the namespace specified in the command line.

so in this PR:

  1. The configuration for namespace has been removed from values.yaml.
  2. When creating all Kubernetes resources, specify the value to use for the namespace as {{ .Release.Namespace }}.

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)?

I confirmed the correctness of the execution result through helm template --dry-run.

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@CLAassistant
Copy link

CLAassistant commented Nov 14, 2023

CLA assistant check
All committers have signed the CLA.

@baiyangtx
Copy link
Contributor Author

baiyangtx commented Nov 14, 2023

Install with -n

$ helm template --dry-run instance  -n middleware   ./seata-server 
---
# Source: seata-server/templates/service.yaml
apiVersion: v1
kind: Service
metadata:
  namespace: "middleware"
  name: instance-seata-server
  labels:
    app.kubernetes.io/name: seata-server
    helm.sh/chart: seata-server-1.0.0
    app.kubernetes.io/instance: instance
    app.kubernetes.io/version: "1.0"
    app.kubernetes.io/managed-by: Helm
spec:
  type: NodePort
  ports:
    - port: 30091
      targetPort: http
      protocol: TCP
      name: http
  selector:
    app.kubernetes.io/name: seata-server
    app.kubernetes.io/instance: instance

Install without -n

$ helm template --dry-run instance    ./seata-server 
---
# Source: seata-server/templates/service.yaml
apiVersion: v1
kind: Service
metadata:
  namespace: "default"
  name: instance-seata-server
  labels:
    app.kubernetes.io/name: seata-server
    helm.sh/chart: seata-server-1.0.0
    app.kubernetes.io/instance: instance
    app.kubernetes.io/version: "1.0"
    app.kubernetes.io/managed-by: Helm
spec:
  type: NodePort
  ports:
    - port: 30091
      targetPort: http
      protocol: TCP
      name: http
  selector:
    app.kubernetes.io/name: seata-server
    app.kubernetes.io/instance: instance



Copy link

codecov bot commented Nov 14, 2023

Codecov Report

Merging #6029 (b7c2840) into develop (c829e73) will decrease coverage by 0.02%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #6029      +/-   ##
=============================================
- Coverage      48.88%   48.86%   -0.02%     
+ Complexity      4186     4185       -1     
=============================================
  Files            793      793              
  Lines          28028    28028              
  Branches        3423     3423              
=============================================
- Hits           13702    13697       -5     
- Misses         12897    12901       +4     
- Partials        1429     1430       +1     

see 3 files with indirect coverage changes

@leizhiyuan
Copy link
Contributor

New features and optimizations should be submitted for merging into the 2.x branch.

@baiyangtx baiyangtx changed the base branch from develop to 2.x November 15, 2023 08:10
@baiyangtx baiyangtx changed the base branch from 2.x to develop November 15, 2023 08:10
@baiyangtx
Copy link
Contributor Author

I see. I will resubmit a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants