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.

Sign up for GitHub

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

Jump to bottom

feat(key-management-service): add kms dashboard #11664

Merged
merged 5 commits into from May 23, 2024
Merged

feat(key-management-service): add kms dashboard #11664

merged 5 commits into from May 23, 2024

Conversation

vovh
Copy link
Contributor

@vovh vovh commented May 7, 2024 •
edited

ref: MANAGER-13567

Question Answer
Branch? feat/key-management-service
Bug fix? no
New feature? yes
Breaking change? no
Tickets Fix #MANAGER-13567
License BSD 3-Clause
  • Try to keep pull requests small so they can be easily reviewed.
  • Commits are signed-off
  • Only FR translations have been updated
  • Branch is up-to-date with target branch
  • Lint has passed locally
  • Standalone app was ran and tested locally
  • Ticket reference is mentioned in linked commits (internal only)
  • Breaking change is mentioned in relevant commits

Description

As a customer, I want to view KMS details, and make actions on it (edit information and encryption methods)

Related

@github-actions github-actions bot added the feature New feature label May 7, 2024
@vovh vovh force-pushed the feat/key-management-service branch from 1c56612 to 415a228 Compare May 14, 2024 12:38
@github-actions github-actions bot added has conflicts Has conflicts to resolve before merging labels May 14, 2024
@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label May 14, 2024
@github-actions github-actions bot removed dependencies Pull requests that update a dependency file has conflicts Has conflicts to resolve before merging labels May 14, 2024
@vovh vovh force-pushed the feat/key-management-service branch from 415a228 to cc4333f Compare May 14, 2024 13:12
@github-actions github-actions bot added the has conflicts Has conflicts to resolve before merging label May 14, 2024
@github-actions github-actions bot removed the has conflicts Has conflicts to resolve before merging label May 14, 2024
@vovh vovh force-pushed the feat/key-management-service branch from cc4333f to a1ffd47 Compare May 14, 2024 15:05
@github-actions github-actions bot added the has conflicts Has conflicts to resolve before merging label May 14, 2024
@chipp972 chipp972 force-pushed the feat/key-management-service branch from a1ffd47 to 403c833 Compare May 15, 2024 08:19
@github-actions github-actions bot removed the has conflicts Has conflicts to resolve before merging label May 15, 2024
@vovh vovh force-pushed the feat/key-management-service branch from eae9871 to 236ad23 Compare May 15, 2024 14:30
@vovh vovh force-pushed the feat/MANAGER-13567 branch 4 times, most recently from ce9e006 to f25d5ee Compare May 20, 2024 13:08
@vovh vovh self-assigned this May 21, 2024
@vovh vovh marked this pull request as ready for review May 21, 2024 13:03
@vovh vovh requested review from a team as code owners May 21, 2024 13:03
lionel95200x
lionel95200x reviewed May 22, 2024
View reviewed changes
packages/manager/apps/key-management-service/src/components/Listing/ListingCells.tsx
return <DataGridTextCell>{props.iam.displayName}</DataGridTextCell>;
const navigate = useNavigate();
return (
<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

You could remove this div

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't, alex gonna talk about it on react sharing but if we remove it, it makes the browser crash. Probably some issue with ODS Link and the onClick

Copy link
Contributor

Choose a reason for hiding this comment

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

we have to find why we need to wrap with a div ? Nicolas has the same issue

lionel95200x
lionel95200x reviewed May 22, 2024
View reviewed changes
packages/manager/apps/key-management-service/src/components/Modal/Modal.tsx
import { OsdsModal } from '@ovhcloud/ods-components/react';
import React, { useRef } from 'react';

const Modal: React.FC<React.PropsWithChildren & {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you could move this component to manager-components cause it's a duplicate packages/manager/apps/pci-rancher/src/components/Modal/Modal.tsx

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 need to create a task or plan it in refinement.
I think there is already work ongoing for edit and deletion.
Gonna refactor when it's up i'll just keep it this way for now and refactor when available.

Copy link
Contributor

Choose a reason for hiding this comment

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

already in progress in another PR 👍

lionel95200x
lionel95200x reviewed May 22, 2024
View reviewed changes
packages/manager/apps/key-management-service/src/components/Modal/EditNameModal.tsx
onEditName: (okms: OKMS) => void;
}

const EditNameModal = ({ okms, toggleModal, onEditName }: DeleteModalProps) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it will be better if you only pass the name here instead of full okms object so you can compute your onEditName in parent to have a reusable Modal :)

Copy link
Contributor

Choose a reason for hiding this comment

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

It will be rework with a generic component 👍

@darsene darsene force-pushed the feat/key-management-service branch from 9a4c108 to d5108ca Compare May 22, 2024 12:58
@github-actions github-actions bot added the has conflicts Has conflicts to resolve before merging label May 22, 2024
vovh added 3 commits May 22, 2024 15:05
@vovh @darsene
feat(kms): refacto into key-management-service name
8e44945 
ref: MANAGER-14018

Signed-off-by: Vincent BONMARCHAND <vincent.bonmarchand.ext@corp.ovh.com>
@vovh @darsene
feat(key-management-service): add kms onboarding page
041adab 
ref: MANAGER-13826

Signed-off-by: Vincent BONMARCHAND <vincent.bonmarchand.ext@corp.ovh.com>
@vovh @darsene
feat(key-management-service): add kms dashboard
aa39e1f 
ref: MANAGER-12567

Signed-off-by: Vincent BONMARCHAND <vincent.bonmarchand.ext@corp.ovh.com>
@github-actions github-actions bot added enabler and removed has conflicts Has conflicts to resolve before merging labels May 22, 2024
@aboungnaseng-ovhcloud @vovh
feat(key-management-service): fix display name link in datagrid
01551c7 
ref: MANAGER-12567

Signed-off-by: Alex Boungnaseng <alex.boungnaseng.ext@corp.ovh.com>
Signed-off-by: David Arsène <david.arsene.ext@ovhcloud.com>
@vovh vovh requested a review from darsene May 22, 2024 15:11
@vovh vovh requested a review from lionel95200x May 22, 2024 15:23
@darsene
feat(key-management-service): fix review feedback
004a3dc 
ref: MANAGER-13567

Signed-off-by: David Arsène <david.arsene.ext@ovhcloud.com>
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented May 23, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@darsene darsene merged commit 21a156f into feat/key-management-service May 23, 2024
13 of 14 checks passed
@darsene darsene deleted the feat/MANAGER-13567 branch May 23, 2024 14:39
darsene pushed a commit that referenced this pull request May 24, 2024
@vovh @darsene
feat(key-management-service): add kms dashboard (#11664)
78353c5 
ref: MANAGER-13567

Signed-off-by: Vincent BONMARCHAND <vincent.bonmarchand.ext@corp.ovh.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darsene darsene darsene approved these changes

@aboungnaseng-ovhcloud aboungnaseng-ovhcloud aboungnaseng-ovhcloud approved these changes

@sidlynx sidlynx Awaiting requested review from sidlynx sidlynx is a code owner automatically assigned from ovh/su-digital-tools-core-stack

@kqesar kqesar Awaiting requested review from kqesar kqesar is a code owner automatically assigned from ovh/su-digital-tools-core-stack

@antonymarion antonymarion Awaiting requested review from antonymarion antonymarion is a code owner automatically assigned from ovh/team-manager-dev

@sachinrameshn sachinrameshn Awaiting requested review from sachinrameshn sachinrameshn is a code owner automatically assigned from ovh/team-manager-dev

@lionel95200x lionel95200x Awaiting requested review from lionel95200x

Assignees

@vovh vovh

Labels
enabler feature New feature translation required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@vovh @lionel95200x @darsene @aboungnaseng-ovhcloud