-
-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
Allow users to delete AppNamespace #4499
Conversation
# Conflicts: # README.md
Codecov Report
@@ Coverage Diff @@
## master #4499 +/- ##
============================================
- Coverage 53.46% 53.45% -0.02%
- Complexity 2703 2704 +1
============================================
Files 490 491 +1
Lines 15347 15395 +48
Branches 1598 1599 +1
============================================
+ Hits 8205 8229 +24
- Misses 6585 6607 +22
- Partials 557 559 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
if (!checkBranchInstance(toDeleteNamespace)) { | ||
return; | ||
} | ||
if(toDeleteNamespace.isLinkedNamespace){ |
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.
For public and private namespaces, I think we still need to check the instances bycheckMasterInstance
and checkBranchInstance
?
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.
- Here it is considered that the user delete action is expected.
- Only the current environment and the usage of the current cluster can be checked here, and deleting the AppNamespace will affect all clusters, so the logic here remains consistent with the administrator's deletion
- Or, is there a need to check the usage of all clusters in all environments?
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.
In this case, there is a detailed message indicating that this operation will cause the instances unable to get the configuration, so I think it's ok to not checkMasterInstance
and checkBranchInstance
.
If we don't check for public and private namespaces, I suppose we don't need to check for linked namespaces?
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.
If we don't check for public and private namespaces, I suppose we don't need to check for linked namespaces?
Yes, I think they are the same type of problem
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.
So maybe we could simplify the logic as the following?
//1. check operator has master permission
checkPermission(toDeleteNamespace).then(function () {
if (!toDeleteNamespace.isPublic || toDeleteNamespace.isLinkedNamespace) {
showDeleteNamespaceConfirmDialog();
} else {
//2. check public namespace has not associated namespace
checkPublicNamespace(toDeleteNamespace).then(function () {
showDeleteNamespaceConfirmDialog();
});
}
})
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.
apollo-portal/src/main/resources/static/scripts/directive/delete-namespace-modal-directive.js
Show resolved
Hide resolved
Co-authored-by: Jason Song <nobodyiam@gmail.com>
Co-authored-by: Jason Song <nobodyiam@gmail.com>
Co-authored-by: Jason Song <nobodyiam@gmail.com>
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.
It looks great! Please find some comments below.
...o-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceController.java
Outdated
Show resolved
Hide resolved
...o-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/NamespaceController.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/AppNamespaceService.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/directive/delete-namespace-modal-directive.js
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/directive/delete-namespace-modal-directive.js
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/directive/delete-namespace-modal-directive.js
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/directive/delete-namespace-modal-directive.js
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/directive/delete-namespace-modal-directive.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Jason Song <nobodyiam@gmail.com>
Co-authored-by: Jason Song <nobodyiam@gmail.com>
…te-namespace-modal-directive.js Co-authored-by: Jason Song <nobodyiam@gmail.com>
…te-namespace-modal-directive.js Co-authored-by: Jason Song <nobodyiam@gmail.com>
…te-namespace-modal-directive.js Co-authored-by: Jason Song <nobodyiam@gmail.com>
…te-namespace-modal-directive.js Co-authored-by: Jason Song <nobodyiam@gmail.com>
…te-namespace-modal-directive.js Co-authored-by: Jason Song <nobodyiam@gmail.com>
…controller/NamespaceController.java Co-authored-by: Jason Song <nobodyiam@gmail.com>
…controller/NamespaceController.java Co-authored-by: Jason Song <nobodyiam@gmail.com>
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
What's the purpose of this PR
Consistent with user deletion of Namespace and administrator deletion of AppNamespace