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

fix: Object Storage Object URLs #10603

Merged

Conversation

bnussman-akamai
Copy link
Member

Description 📝

  • Fixes incorrect Object Storage URLs given to users by Cloud Manager when Multi-Cluster is enabled

The Issue 🐛

  • Cloud Manager was putting the region id in the URL instead of the expected cluster id

Preview 📷

Before After
Screen.Recording.2024-06-21.at.3.30.21.PM.mov
Screen.Recording.2024-06-21.at.3.29.41.PM.mov

How to test 🧪

  • Make sure you have an Object Storage bucket on your account
  • Using an existing object (or upload a new object), open that object's details drawer
  • Verify the link in that details drawer brings you to the object 🔗

As an Author I have 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

@bnussman-akamai bnussman-akamai added Hotfix Hotfix: This is going to staging OBJ Multi-Cluster labels Jun 21, 2024
@bnussman-akamai bnussman-akamai self-assigned this Jun 21, 2024
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner June 21, 2024 19:35
@bnussman-akamai bnussman-akamai requested review from carrillo-erik and cpathipa and removed request for a team June 21, 2024 19:35
@bnussman-akamai bnussman-akamai changed the base branch from develop to staging June 21, 2024 19:35
Copy link

github-actions bot commented Jun 21, 2024

Coverage Report:
Base Coverage: 82.29%
Current Coverage: 82.29%

@dwiley-akamai dwiley-akamai self-requested a review June 21, 2024 20:13
abailly-akamai
abailly-akamai previously approved these changes Jun 21, 2024
Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Confirmed this fixed the issue on my end with the BucketDetail link

? buckets?.buckets.find(
(bucket) => bucket.label === bucketName && bucket.region === clusterId
)?.cluster ?? ''
: clusterId;
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see how you don't find this ideal - perhaps rendering a separate component if isObjMultiClusterEnabled is true to only fetch in that case can be considered as a refactor follow up

Copy link
Contributor

@cpathipa cpathipa left a comment

Choose a reason for hiding this comment

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

Confirming on the fix. Approving pending tests.

Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Confirmed link pointing to the right object

return bucket.label === bucketName && bucket.region === clusterId;
}
return bucket.label === bucketName && bucket.cluster === clusterId;
});
Copy link
Contributor

Choose a reason for hiding this comment

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

does seem to delay the loading of the URL but there's another loading pattern on the page so i think we're ok

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

Object URL leads to the expected object now ✅
Code review ✅

Copy link
Contributor

@hana-akamai hana-akamai left a comment

Choose a reason for hiding this comment

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

Confirmed link leads to expected object

@bnussman-akamai bnussman-akamai merged commit d1263e7 into linode:staging Jun 21, 2024
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hotfix Hotfix: This is going to staging OBJ Multi-Cluster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants