-
Notifications
You must be signed in to change notification settings - Fork 391
feat: Allow multi-cluster Marketplace deployments #12648
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
feat: Allow multi-cluster Marketplace deployments #12648
Conversation
…size is only defined
dwiley-akamai
left a comment
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.
Hi @n0vabyte, thank you for opening this PR. Where are these changes coming from–a JIRA ticket that outlines the requirements for this work, an independent idea you had and implemented, etc.?
…nager into feature/marketplace-clusters
packages/manager/src/features/Linodes/LinodeCreate/Summary/utilities.ts
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Linodes/LinodeCreate/Summary/Summary.tsx
Outdated
Show resolved
Hide resolved
…mary.tsx Co-authored-by: Banks Nussman <[email protected]>
|
@bnussman-akamai any updates? |
|
Will be reviewing today |
bnussman-akamai
left a comment
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.
Overall functionality is looking solid! I just think we need to tidy up some things
packages/manager/src/features/Linodes/LinodeCreate/Summary/Summary.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Linodes/LinodeCreate/Summary/utilities.ts
Outdated
Show resolved
Hide resolved
…lities.ts Co-authored-by: Banks Nussman <[email protected]>
…mary.tsx Co-authored-by: Banks Nussman <[email protected]>
|
Any updates? |
|
Hoping to rereview soon. Do we have a testing plan for this? I think it would ideal if we have some thing in place. Like unit tests, cypress e2e tests, or a QA team responsible for testing this |
dwiley-akamai
left a comment
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.
Verification steps ✅
I agree that implementing some more robust unit and/or E2E tests or having a testing plan in place would be best
packages/manager/src/features/Linodes/LinodeCreate/Summary/utilities.ts
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Linodes/LinodeCreate/Summary/utilities.ts
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Linodes/LinodeCreate/Summary/Summary.tsx
Outdated
Show resolved
Hide resolved
…nager into feature/marketplace-clusters
|
I'm going to push up one more small change, then I'll approve! |
Cloud Manager UI test results🔺 1 failing test on test run #17 ↗︎
Details
TroubleshootingUse this command to re-run the failing tests: pnpm cy:run -s "cypress/e2e/core/linodes/clone-linode.spec.ts" |
|||||||||||||||||
bnussman-akamai
left a comment
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.
Looks good! I pushed up some changes and unit tests so double check behind me
I think the clone-linode.spec.ts failure can be disregarded
Description 📝
Updates how UDF data
cluster_sizeworks in stackscripts. The goal of this PR is to introduce multi-cluster instance types for the Marketplace apps. Most importantly, reflect the correct price for the cluster being created. This PR allows us to match$prefix_cluster_sizewith$prefix_cluster_typeto create a dynamic cluster in a cluster deployment. For example, ELK can have multi-node cluster with different instance types associated with it.Changes 🔄
packages/manager/src/features/Linodes/LinodeCreate/Summary/utilities.tsupdated to parse cluster data associated with the instance type being created.cluster_nameof $n nodes" is being created.packages/manager/src/features/Linodes/LinodeCreate/utilities.tsis updated to map the instance Label that the customer is created to a valid instance type. I am usingtypes.json.Scope 🚢
Upon production release, changes in this PR will be visible to:
$prefix_cluster_size,$prefix_cluster_typeandcluster_namein their stackscripts to extend functionality.Target release date 🗓️
Next release cycle if possible. No rush on making the changes GA.
Preview 📷
How to test 🧪
Step 1: Create a new Stackscript
Step 2: Deploy StackScript
From here you will be able to select different cluster size for Kibana, Elasticsearch and Logstash. The Linode plan that is selected for Elasticsearch and Logstash is appropriately added to the total at the bottom of the Summary.
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
🧪 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 ✅