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

[#6540] Platform: Remove certificate feature. #6885

Merged
merged 55 commits into from
Feb 11, 2021

Conversation

mahendranbhat
Copy link
Contributor

@mahendranbhat mahendranbhat commented Jan 15, 2021

Description: Remove certificate feature which will be added in the list view of certificates to delete unused certificates.
If the certificate is linked to the existing universe then we will be disabling the button in the FE.

Testing: :

  1. Go to certificates
  2. Try to delete the certificate which is not linked to any universe.
  3. Also we can not see the delete option for certs which is linked to the universe.

sbt run

[info] Passed: Total 1186, Failed 0, Errors 0, Passed 1182, Skipped 4

@CLAassistant
Copy link

CLAassistant commented Jan 15, 2021

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ gaurav061
✅ mahendranbhat
❌ mahendra bhat


mahendra bhat seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor

@Arnav15 Arnav15 left a comment

Choose a reason for hiding this comment

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

Another thing, we use the google style guide for code formatting, which means line breaks need atleast a 4 space indentation

@mahendranbhat mahendranbhat requested a review from Arnav15 January 16, 2021 13:26
Copy link
Contributor

@Arnav15 Arnav15 left a comment

Choose a reason for hiding this comment

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

Also, it is important to correctly add the description as I had mentioned earlier.
Add the sections:
Description:
Testing: How you tested it, and the summary lines of the sbt test command. Should look like:

[info] Passed: Total 1115, Failed 0, Errors 0, Passed 1111, Skipped 4

Copy link
Collaborator

@sshev sshev left a comment

Choose a reason for hiding this comment

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

need some refinements on UI side

managed/ui/src/components/certificates/Certificates.js Outdated Show resolved Hide resolved
managed/ui/src/components/certificates/Certificates.js Outdated Show resolved Hide resolved
managed/ui/src/components/certificates/Certificates.js Outdated Show resolved Hide resolved
managed/ui/src/components/certificates/Certificates.js Outdated Show resolved Hide resolved
managed/ui/src/components/certificates/Certificates.js Outdated Show resolved Hide resolved
managed/ui/src/components/certificates/Certificates.js Outdated Show resolved Hide resolved
managed/ui/src/components/certificates/Certificates.js Outdated Show resolved Hide resolved
@mahendranbhat mahendranbhat marked this pull request as draft January 22, 2021 05:43
@mahendranbhat mahendranbhat changed the title [#6540] Platform: Remove certificate feature. [#6540] [WIP] Platform: Remove certificate feature. Jan 22, 2021
Copy link
Contributor

@Arnav15 Arnav15 left a comment

Choose a reason for hiding this comment

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

Looks good, just these minor fixes and then it should be good to go.

@@ -145,9 +146,29 @@ public Result get(UUID customerUUID, String label) {
}
}

public Result delete(UUID customerUUID, UUID reqCertUUID) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a check to verify the certificate belongs to the customer.


public static boolean existsCertificate(UUID certUUID, UUID customerUUID) {
Set<Universe> universeList = Customer.get(customerUUID).getUniverses();
for (Universe universe : universeList) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Maybe use the java stream functionality. (If unaware, take a look here)

@mahendranbhat mahendranbhat changed the title [#6540] [WIP] Platform: Remove certificate feature. [#6540] Platform: Remove certificate feature. Feb 2, 2021
@mahendranbhat mahendranbhat linked an issue Feb 2, 2021 that may be closed by this pull request
@mahendranbhat mahendranbhat requested a review from Arnav15 February 2, 2021 12:56
@mahendranbhat mahendranbhat marked this pull request as ready for review February 9, 2021 05:39
@gaurav061 gaurav061 merged commit 74bf17e into yugabyte:master Feb 11, 2021
polarweasel pushed a commit to lizayugabyte/yugabyte-db that referenced this pull request Mar 9, 2021
* certificate delete api

* added one field to response

* added removable

* [6540] Added ability to delete certificates

* added removable field

* removable added

* removable added to list api

* [6540]Added ability to remove certificate

* added validation for remove certificates.

* review comment fixes

* replaced removable props with inUse as recommended

* fixed disabled scenario of remove cert button

* review comment fixes

* Addressed review comments

* modified test cases

* Code improvements

* modified FE code as recommended

* removed local state declaration

* Review comment fixes

* review comment fixes

* review comment fixes

* review comment fixes

* review comment fixes

* certificate delete api

* added one field to response

* added removable

* [6540] Added ability to delete certificates

* added removable field

* removable added

* [6540]Added ability to remove certificate

* removable added to list api

* added validation for remove certificates.

* review comment fixes

* replaced removable props with inUse as recommended

* fixed disabled scenario of remove cert button

* review comment fixes

* Addressed review comments

* modified test cases

* Code improvements

* modified FE code as recommended

* removed local state declaration

* Review comment fixes

* review comment fixes

* review comment fixes

* review comment fixes

* review comment fixes

* review comment fixes

* Review comment fixes

* Review comment fixes

Co-authored-by: mahendra bhat <[email protected]>
Co-authored-by: gaurav061 <[email protected]>
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.

Add ability to remove certificates from UI
6 participants