new: STORIF-320 - Object storage creation flow updated.#13508
new: STORIF-320 - Object storage creation flow updated.#13508dchyrva-akamai wants to merge 1 commit intolinode:developfrom
Conversation
dchyrva-akamai
force-pushed
the
feature/STORIF-320
branch
4 times, most recently
from
b33e9c9 to
dc1fbdb
Compare
dchyrva-akamai
force-pushed
the
feature/STORIF-320
branch
3 times, most recently
from
0de6612 to
fc5387f
Compare
| export const grantTypeMap = { | ||
| account: 'Account', | ||
| bucket: 'Buckets', | ||
| key: 'Access Keys', |
There was a problem hiding this comment.
I'm not sure if we should add grant mapping for keys if there are no key grants at the moment. I understand that this is for getRestrictedResourceText method, but maybe it would be better to provide an explicit tooltip text for the Access Key button for now, e.g. "You must have full account access to perform this operation.".
We will need some input from the Cloud Manager core team on this one.
There was a problem hiding this comment.
I just discussed it with Jason - we should use a dedicated text for both Buckets and Access Key. Jason sugested the following text:
You don't have administrator privileges for Object Storage. Your administrator must change your account to full access to provide access to Object Storage.
We can improve the text later after discussing it with TW.
| let drawerUrl = `/object-storage/access-keys/${objectStorageKey.id}`; | ||
|
|
||
| if (mode === 'editing') { | ||
| drawerUrl += `/update`; |
There was a problem hiding this comment.
Do we really need this? To make it consistent, we would need to have URL for each action. Looks like it's out of the scope for the current ticket.
There was a problem hiding this comment.
"we would need to have URL for each action", yes, this is the pattern we are using for Volumes, and I made the first step to migrate OBJ towards that same pattern.
"out of the scope for the current ticket.", refactoring yes, but for the landing page to work correctly I had to make AccessKeys and CreateBucket drawers to be standalone. It is a relatively small change in my opinion.
| segment={`${accessDrawerOpen ? `Create an Access Key` : `Access Keys`}`} | ||
| /> | ||
| <> | ||
| <DocumentTitleSegment segment="Access Keys" /> |
There was a problem hiding this comment.
This change create inconsistency. In all other pages, the title says "Create a ..." when on the create page.
There was a problem hiding this comment.
To be honest, I don't like the way it was initially implemented. I did the simplification and added a TODO comment to create a proper component for displaying those titles.
Do you think we can leave it like this for now (there is no Create a title) and create a refactoring ticket?
There was a problem hiding this comment.
Do we need ObjectStorage prefix if it's in the ObjectStorage package?
There was a problem hiding this comment.
https://www.folger.edu/explore/shakespeares-works/hamlet/read/3/1/#line-3.1.64
I was following the same pattern as we are using for Volumes. If it's worth it, maybe we could refactor both later.
There was a problem hiding this comment.
I think we should avoid organizing components by their type and do that rather by their domain or specific functionality, e.g. Buckets, AccessKeys, etc; on a more granular level AccessKeyTable, AccessKeyDetails, etc.
There was a problem hiding this comment.
I treat drawers as separate type here because they might be reused in some other parts of the application, and I am trying to bring ObjectStorage to the same pattern that we are using for Volumes:
https://github.com/linode/manager/tree/develop/packages/manager/src/features/Volumes/VolumeDrawers
| .sort(sortByCluster); | ||
|
|
||
| export const AccessKeyDrawer = (props: AccessKeyDrawerProps) => { | ||
| export const AccessKeyDrawerV1 = (props: AccessKeyDrawerProps) => { |
There was a problem hiding this comment.
If we really need multiple versions of the components, the new components should get a suffix or prefix, not the old one. That would make it much more easy to identify what's changed for someone familiar with the existing code.
There was a problem hiding this comment.
Reverted this change.
Initially, I created a proxy component AccessKeyDrawer with both AccessKeyDrawer and OMC_AccessKeyDrawer inside and added the version to avoid naming conflict, but later it got renamed into AccessKeyDrawers so version is not needed anymore.
|
The sheer number of changes is terrifying... Unfortunately, I wasn't able to go through all the changes in a single session. |
dchyrva-akamai
force-pushed
the
feature/STORIF-320
branch
4 times, most recently
from
4776a97 to
b7c59a3
Compare
dchyrva-akamai
force-pushed
the
feature/STORIF-320
branch
from
b7c59a3 to
1ba6298
Compare
Cloud Manager UI test results🔺 10 failing tests on test run #14 ↗︎
Details
TroubleshootingUse this command to re-run the failing tests: pnpm cy:run -s "cypress/e2e/core/kubernetes/lke-create.spec.ts,cypress/e2e/core/objectStorage/object-storage.smoke.spec.ts,cypress/e2e/core/objectStorage/object-storage.e2e.spec.ts,cypress/e2e/core/objectStorageMulticluster/bucket-create-multicluster.spec.ts,cypress/e2e/core/objectStorageMulticluster/object-storage-objects-multicluster.spec.ts,cypress/e2e/core/linodes/alerts-create.spec.ts,cypress/e2e/core/objectStorage/enable-object-storage.spec.ts,cypress/e2e/core/objectStorageMulticluster/bucket-delete-multicluster.spec.ts" |
||||||||||||||||||||||||||||||||||||||||||||
|
After looking into the problem more carefully I still thing that the pull request is too big and cover too many concerns, some of which were not covered in the original ticket, occasionally the complexity has risen (like now we have 3 components for Access Keys Drawer instead of 2), and the code was too strongly reorganized, while there were no need for it. Contributing guidelines are clear on this one: a good PR is small. In order to avoid discussion running in several direction in this PR, I'd like to separate the concerns covered in this ticket. A first step to it will be creating a separate ticket to refactor the drawer management for Object Storage, which should be a prerequisite for reorganizing the "empty" state landing page. I know this might seem painful, but sometimes you need to do few steps back to move forward. |
3 participants
Description 📝
Object storage creation flow updated.
Changes 🔄
Preview 📷
How to test 🧪
Delete all the buckets and keys from your account.
Cancel object storage.
Navigate to the
/object-storagepage.Observe OBJ Storage Landing page.
Create a bucket or a key.
Observe landing page get's replace by tabs view.
Author Checklists
As an Author, to speed up the review process, I considered 🤔
👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support
As an Author, before moving this PR from Draft to Open, I confirmed ✅