Skip to content
This repository has been archived by the owner on Jul 2, 2024. It is now read-only.

EVG-19948: Support EC2 On-Demand on provider settings page #2071

Merged
merged 6 commits into from
Sep 29, 2023

Conversation

minnakt
Copy link
Contributor

@minnakt minnakt commented Sep 28, 2023

EVG-19948

Description

Adds support for EC2 On-Demand. It's very similar to the EC2 Fleet settings but doesn't have the fleet options section.

I moved stuff around with the schema & uiSchema in an attempt to make it easier to cross-reference them.

Screenshots

Screenshot 2023-09-28 at 3 51 22 PM

Testing

  • Jest and Cypress tests

Evergreen PR

@cypress
Copy link

cypress bot commented Sep 28, 2023

Passing run #12987 ↗︎

0 595 7 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Last tweaks
Project: Spruce Commit: fb25c9a96a
Status: Passed Duration: 18:39 💡
Started: Sep 29, 2023 8:54 PM Ended: Sep 29, 2023 9:13 PM

Review all test suite changes for PR #2071 ↗︎

@minnakt
Copy link
Contributor Author

minnakt commented Sep 28, 2023

evergreen retry

@minnakt minnakt marked this pull request as ready for review September 29, 2023 14:06
@minnakt minnakt requested a review from a team September 29, 2023 14:06
Copy link
Contributor

@sophstad sophstad left a comment

Choose a reason for hiding this comment

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

Nice, just some tiny notes! Appreciate you breaking out the schema into smaller chunks.

},
},
},
ec2OnDemandProviderSettings: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how fixable this is (or if it needs fixing) but when I switch to EC2 on demand from another provider the card title for the first region is blank

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no problemo, just gotta tweak the transformer :)

amiId,
instanceType,
sshKeyName,
instanceProfileARN,
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] The legacy platform mentions that ARN = Amazon Resource Name, so maybe that would be worth including in the description

cy.contains("Default VPC Subnet ID").should("exist");
cy.contains("VPC Subnet Prefix").should("exist");

cy.dataCy("use-vpc").uncheck({ force: true });
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's reliable to use force when making assertions on components related to the test description. Using force prevents testing if the use-vpc button is actually available to click. If use-vpc is disabled or hidden, this code will pass through. I think force usage is okay when setting the page up in a particular state to begin testing because those assertions are less important then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using force is pretty much required for LG checkboxes since they have a styling that makes them hidden and unclickable

Comment on lines +181 to +182
cy.contains("Default VPC Subnet ID").should("not.exist");
cy.contains("VPC Subnet Prefix").should("not.exist");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to take advantage asserting visibility if showing/hiding is a theme of the test.

cy.get("@keyNameInput").type("my ssh key");
cy.getInputByLabel("User Data").type("<powershell></powershell>");
cy.getInputByLabel("Merge with existing user data").check({
force: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably better to not use force here since this test is about form interaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same comment as above

force: true,
});
save();
cy.validateToast("success");
Copy link
Contributor

Choose a reason for hiding this comment

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

It's helpful to include the toast copy because it helps document the test and leaves a breadcrumb to the hook.

@minnakt minnakt merged commit 7462229 into evergreen-ci:main Sep 29, 2023
2 checks passed
@minnakt minnakt deleted the EVG-19948 branch September 29, 2023 21:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants