EVG-19947: Support EC2 Fleet on provider settings page #2050
Conversation
Passing run #12912 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
||||||||||||||||||||
| size: { | ||
| type: "number" as "number", | ||
| title: "Size", | ||
| }, |
There was a problem hiding this comment.
I think size is required if virtualName is empty. You could maybe use a bidirectional dependency
There was a problem hiding this comment.
I think the bidirectional dependencies don't work with the ! condition (?) so I gave this a try using anyOf with required but it didn't work... I also tried using the validate() function prop but I don't have access to rawErrors on the array template until v5 🙃
Sadly I think I may just have to leave it as it is, but I checked and made sure the backend does a validation error if both fields aren't defined
|
|
||
| const useCapacityOptimization = { | ||
| type: "boolean" as "boolean", | ||
| title: |
There was a problem hiding this comment.
[nit] Could probably make this more scannable by making the title "Capacity optimaztion" (maybe bolded?) and including the rest as a description
| userData: { | ||
| "ui:widget": "textarea", | ||
| "ui:elementWrapperCSS": textAreaCSS, | ||
| }, |
There was a problem hiding this comment.
[nit] If you want, you could probably situate the "Merge with existing user data" checkbox on the top-right of the texarea with some CSS like I did here
| region: { | ||
| "ui:data-cy": "region-select", | ||
| "ui:allowDeselect": false, | ||
| "ui:enumDisabled": fleetRegions, |
| GET_AWS_REGIONS | ||
| ); | ||
| const { awsRegions } = awsData || {}; | ||
| const fleetRegions = formData?.ec2FleetProviderSettings?.map((p) => p.region); |
There was a problem hiding this comment.
[nit] It could be useful to name this something like configuredRegions to indicate it's based on the distro data, not just the available options
| subnetId: { | ||
| "ui:placeholder": "e.g. subnet-xxxx", | ||
| }, | ||
| subnetPrefix: { | ||
| "ui:description": | ||
| "Will look for subnets like <prefix>.subnet_1a, <prefix>.subnet_1b, etc.", | ||
| }, |
There was a problem hiding this comment.
[nit] You could indent these fields if you want to indicate they're associated with the checkmark above
|
|
||
| // Change field values. | ||
| cy.selectLGOption("Region", "us-west-1"); | ||
| cy.getInputByLabel("SSH Key Name").clear(); |
There was a problem hiding this comment.
| cy.getInputByLabel("SSH Key Name").clear(); | |
| cy.getInputByLabel("SSH Key Name").as("keyNameInput").clear(); |
| // Change field values. | ||
| cy.selectLGOption("Region", "us-west-1"); | ||
| cy.getInputByLabel("SSH Key Name").clear(); | ||
| cy.getInputByLabel("SSH Key Name").type("my ssh key"); |
There was a problem hiding this comment.
| cy.getInputByLabel("SSH Key Name").type("my ssh key"); | |
| cy.get("@keyNameInput").type("my ssh key"); |
| cy.getInputByLabel("SSH Key Name").clear(); | ||
| cy.getInputByLabel("SSH Key Name").type("mci"); |
There was a problem hiding this comment.
| cy.getInputByLabel("SSH Key Name").clear(); | |
| cy.getInputByLabel("SSH Key Name").type("mci"); | |
| cy.get("@keyNameInput").clear(); | |
| cy.get("@keyNameInput").type("mci"); |
| cy.dataCy("delete-item-button").click(); | ||
| }); | ||
| save(); | ||
| cy.validateToast("success"); |
There was a problem hiding this comment.
It'll be great to validate the toast copy in the second parameter
There was a problem hiding this comment.
I think I want to leave this one as-is since I would have to update multiple other test files for consistency (also I'm not sure that checking the copy is too important, it says the same thing every time)
There was a problem hiding this comment.
Sorry I didn't explain my reasoning fully. A benefit of being specific is to help with maintaining and debugging the test suite. If an issue occurs where the toast isn't dispatched, including the toast message allows to easily search the codebase for where the toast should have been dispatched. Even though the toast says the same thing every time for this test suite, we dispatch over 60 different success messages in Spruce. I hear you out about updating multiple files and the repetition in checking copy. I think ideally validateToast or it's usage should encapsulate the toast copy so we don't need to pass the parameter explicitly for every toast validation statement. It's okay to leave this as is for now but eventually I'll like to clean this up across our Cypress tests in a separate ticket: https://jira.mongodb.org/browse/EVG-20920
| cy.dataCy("capacity-optimization").should("not.exist"); | ||
|
|
||
| cy.selectLGOption("Fleet Instance Type", "Spot"); | ||
| cy.dataCy("use-capacity-optimization").should("exist"); |
There was a problem hiding this comment.
If the element is visible, it's more robust to assert should("be.visible") since it must exist to visible and also because the test is about showing and hiding.
| // Correct section is displayed. | ||
| cy.dataCy("ec2-fleet-provider-settings").should("exist"); | ||
| cy.dataCy("ec2-fleet-provider-settings") | ||
| .children() |
There was a problem hiding this comment.
This test will be more robust if it doesn't count the children HTML element and instead uses contains to assert existence. A package or code change could introduce a wrapper component that could always make this statement true and being explicit about what is contained will make the test more expressive so it's possible to understand what's contained under [data-cy="ec2-fleet-provider-settings" ] by reading the test code.
EVG-19947
Description
This PR adds support for EC2 Fleet.
Initially, I was going to make the regions static as indicated by the designs. This would mean showing an item for each of the AWS regions by default, even if there are no settings that correspond to that region.
But then I found out that the backend validation doesn't allow incomplete entries in the provider settings list — that is, if there's an item corresponding to
"us-west-1", then all of its required fields must be filled out. This means that I would have to filter out the "empty" regions before sending the output to GQL, but it's not easy to determine what makes a region empty.I changed the design so that you add a new item whenever you want to specify settings for a new region. This works better with the backend validation since it requires the region settings to be properly filled out when you save the page. Design-wise I think the change is fine and might better at reducing clutter on the page.
Screenshots
Testing