manager icon indicating copy to clipboard operation
manager copied to clipboard

upcoming: [APL-55] - Application platform for Linode Kubernetes

Open dennisvankekem opened this issue 1 year ago • 1 comments

Description 📝

UI implementation for the upcoming APL ( Application platform for Linode) feature

Changes 🔄

List any change relevant to the reviewer.

  • APL UI elements added to the createKubernetes and KubernetesOverview pages
  • HA mode will automatically be set whenever APL is enabled
  • Kubernetes plans that are too small will be grayed out whenever APL is enabled
  • Breadcrumb is extended so it support a array of numbers

Target release date 🗓️

N/A

Preview 📷

https://github.com/user-attachments/assets/9a5a1fcd-12bc-4cbd-8414-8552f1a61b84

image

How to test 🧪

  • coming soon

dennisvankekem avatar Aug 06 '24 09:08 dennisvankekem

please add the 'do not merge' and 'work in progress' tags

dennisvankekem avatar Aug 06 '24 09:08 dennisvankekem

This PR is stale because it has been open 15 days with no activity. Please attend to this PR or it will be closed in 5 days

github-actions[bot] avatar Aug 31 '24 00:08 github-actions[bot]

@dennisvankekem is this PR still relevant or a work in progress? I am going to help move things with APL and wondering about the state of this contribution. If moving forward with it, can we get it rebased and updated? I'll start reviewing the code then - thx!

abailly-akamai avatar Sep 05 '24 16:09 abailly-akamai

@abailly-akamai I will update this PR with some final minor changes and then the code is ready to test. To test the functionality of the feature however we have to wait until the APL feature in API is merged first. I will rebase and mark this PR for review once I've pushed the last changes which is expected somewhere tomorrow

dennisvankekem avatar Sep 10 '24 14:09 dennisvankekem

@abailly-akamai Thanks for your review, sadly I didn't get notified by Github in my email for some reason, so your review wasn't noticed by me. I will look at it tomorrow!

dennisvankekem avatar Oct 02 '24 14:10 dennisvankekem

Changes:

  • Split conditional api_root call into separate functions for
    • GetKubernetesCluster
    • CreateKubernetesCluster
  • Removed Apl_enabled from types and instead extended said types with dedicated beta type
  • Created AplAvailability hook where the beta flag logic is handled
  • Made AplEnabled required and removed the possibility of being undefined
  • CreateCluster.tsx now has conditional logic based on flag to use beta features.
  • createCluster function itself has been rewritten to handle calls conditional to avoid duplicated functions. (This function also gets send to child components and I do not want that to cascade in more duplicated code)
  • Change const APL to apl_enabled as the name APL for state might be confusing in what it does
  • Removed console.error from APLSummary.tsx

One major problem is that with this current solution a lot of Kubernetes functions needed to be duplicated. Although the damage for create functions is somewhat controlled, checking for is_apl_enabled in ClusterOverview would borderline cascade into an entire duplication of all Kubernetes functions just for one parameter. With the main culprit being:

export const useKubernetesClusterQuery = (id: number) => {
 return useQuery<KubernetesCluster, APIError[]>(kubernetesQueries.cluster(id));
};

Calling for kubernetesQueries which again cascades into all Kubernetes queries, which are in return strongly typed and therefore cannot easily be conditional rendered based on showAPL flag.

Please let me know what to do with this, perhaps there is a smart solution for this?

dennisvankekem avatar Oct 03 '24 12:10 dennisvankekem

@dennisvankekem thx for addressing this initial feedback - I will give it another go and see what we can come up with to avoid too much duplication

abailly-akamai avatar Oct 03 '24 13:10 abailly-akamai

@dennisvankekem in the interest of time we may want to loosen the typing grip based on the refactor and duplication needed. I did not expect things to cascade that far and I agree the debt may trump the original intent.

Am thinking we could do:

export interface CreateKubeClusterPayload {
  label?: string; // Label will be assigned by the API if not provided
  region?: string; // Will be caught by Yup if undefined
  node_pools: CreateNodePoolData[];
  k8s_version?: string; // Will be caught by Yup if undefined
  control_plane?: ControlPlaneOptions;
  // an important comment about this type, such as this needs a value for BETA
  apl_enabled: boolean | undefined
}

which is not idea but would get us to a compromise where we are always reminder to declare the property. Thoughts?

Meanwhile Can you:

  • fix the current type issues
  • fix any broken test

also, you probably to conditionaly render copy referring to APL. ex (i don't think we want that copy for non-adopters since APL isn't a thing to them): Screenshot 2024-10-03 at 16 47 46

abailly-akamai avatar Oct 03 '24 21:10 abailly-akamai

Coverage Report:
Base Coverage: 87.22%
Current Coverage: 87.23%

github-actions[bot] avatar Oct 04 '24 14:10 github-actions[bot]

'work in progress' and 'do not merge' labels may be removed

dennisvankekem avatar Oct 04 '24 14:10 dennisvankekem

✅ Payload is looking good

Non-blocking: UX needs to consider the tablet/mobile breakpoints for this. As of right now, the UI does not look great at these sizes.

Screenshot 2024-10-08 at 10 54 41 AM

jaalah-akamai avatar Oct 08 '24 14:10 jaalah-akamai

Cloud Manager E2E    Run #6641

Run Properties:  status check passed Passed #6641  •  git commit c0d8900fb3: upcoming: [APL-55] - Application platform for Linode Kubernetes (#10753)
Project Cloud Manager E2E
Run status status check passed Passed #6641
Run duration 25m 28s
Commit git commit c0d8900fb3: upcoming: [APL-55] - Application platform for Linode Kubernetes (#10753)
Committer Dennis van Kekem
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 2
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 429

cypress[bot] avatar Oct 08 '24 18:10 cypress[bot]