-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
Conversation
This is the resubmitted PR of #6029 |
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! |
Codecov Report
Additional details and impacted files@@ 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 |
@@ -1,7 +1,5 @@ | |||
replicaCount: 1 | |||
|
|||
namespace: default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove it?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this 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.
Done.It's ready to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Ⅰ. Describe what this PR did
-n
parameter in thehelm install
command, but currently, deployment ignores the namespace specified in the command line.so in this PR:
namespace
has been removed from values.yaml.{{ .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
Install without
-n
Ⅴ. Special notes for reviews