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 #6034

Merged
merged 9 commits into from
Dec 6, 2023

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

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

Ⅴ. Special notes for reviews

@baiyangtx
Copy link
Contributor Author

@leizhiyuan

This is the resubmitted PR of #6029

@funky-eyes funky-eyes added this to the 2.x Backlog milestone Nov 16, 2023
@baiyangtx
Copy link
Contributor Author

Hello, @funky-eyes I hope you're doing well! Just a friendly reminder that my submitted PR is still awaiting merge. If you have some time, please merge it as soon as possible. Thank you so much for your assistance and patience!

Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Merging #6034 (ec5b113) into 2.x (986fc81) will decrease coverage by 0.05%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #6034      +/-   ##
============================================
- Coverage     49.43%   49.39%   -0.05%     
+ Complexity     4810     4804       -6     
============================================
  Files           913      913              
  Lines         31678    31678              
  Branches       3826     3826              
============================================
- Hits          15661    15647      -14     
- Misses        14477    14484       +7     
- Partials       1540     1547       +7     

see 4 files with indirect coverage changes

@funky-eyes funky-eyes added the first-time contributor first-time contributor label Nov 26, 2023
@funky-eyes funky-eyes modified the milestones: 2.x Backlog, 2.1.0 Nov 26, 2023
@@ -1,7 +1,5 @@
replicaCount: 1

namespace: default
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can apply namespace with command args

helm install instance  -n ${namespace}   ./seata-server 

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can apply namespace with command args

helm install instance  -n ${namespace}   ./seata-server 

ok

Copy link
Contributor

@funky-eyes funky-eyes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please register the author and PR information in the 2.xmd file under the changes folder.

@xingfudeshi
Copy link
Member

Please register the author and PR information in the 2.xmd file under the changes folder.

Done.It's ready to merge.

Copy link
Member

@xingfudeshi xingfudeshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@xingfudeshi xingfudeshi merged commit 9e5abc2 into apache:2.x Dec 6, 2023
8 checks passed
@baiyangtx baiyangtx deleted the helm-using-ns branch December 6, 2023 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants