Modify Cloudability's recommendation action to accept only 30 days look back for S3 type#2253
Modify Cloudability's recommendation action to accept only 30 days look back for S3 type#2253ravikiranvm merged 2 commits intomainfrom
Conversation
|
| 'The look back period in days, used for calculating the recommendations.', | ||
| required: true, | ||
| defaultValue: Duration.TenDay, | ||
| refreshers: ['vendor', 'recommendationType'], |
There was a problem hiding this comment.
have to add these refreshers because the duration dropdown is now dependant of the recommendation type.
| }; | ||
| } | ||
|
|
||
| if (vendor === Vendor.AWS && recommendationType === 's3') { |
There was a problem hiding this comment.
I think this is the simplest approach to add this condition here as I see that all other recommendation types support 10 and 30 days.
There was a problem hiding this comment.
Pull request overview
Updates the Cloudability “Get Recommendations” action to restrict the look-back period options so that AWS S3 recommendations only allow a 30-day lookback (OPS-4202).
Changes:
- Added
getRecommendationDurationProperty()to provide a dynamic duration dropdown based on vendor + recommendation type. - Updated
cloudability_get_recommendationsaction to use the shared duration property. - Added unit tests covering the duration dropdown behavior, including the AWS + S3 restriction.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/blocks/cloudability/src/lib/common/common-properties.ts | Introduces a dynamic duration dropdown and enforces AWS+S3 option list to only 30 days. |
| packages/blocks/cloudability/src/lib/actions/get-recommendations-action.ts | Replaces the previous static duration dropdown with the shared dynamic duration property. |
| packages/blocks/cloudability/test/common-properties.test.ts | Adds tests validating the dynamic duration dropdown state/options, including AWS+S3 restriction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| description: | ||
| 'The look back period in days, used for calculating the recommendations.', | ||
| required: true, | ||
| defaultValue: Duration.TenDay, |
There was a problem hiding this comment.
defaultValue is set to Duration.TenDay, but for AWS + s3 the dropdown options only include Duration.ThirtyDay. This can leave the property in an invalid state (e.g., new steps or existing saved steps defaulting to/retaining ten-day while UI only allows thirty-day). Consider removing the default or switching it to Duration.ThirtyDay and/or ensuring the selected value is coerced when the available options change.
| defaultValue: Duration.TenDay, | |
| defaultValue: Duration.ThirtyDay, |
There was a problem hiding this comment.
I don't see any issues in the UI.
| props: { | ||
| ...getVendorsProperty(), | ||
| ...getRecommendationTypesProperty(), | ||
| duration: Property.StaticDropdown({ | ||
| displayName: 'Look-Back Period', | ||
| description: | ||
| 'The look back period in days, used for calculating the recommendations.', | ||
| required: true, | ||
| defaultValue: Duration.TenDay, | ||
| options: { | ||
| options: [ | ||
| { label: 'Last 10 Days', value: Duration.TenDay }, | ||
| { label: 'Last 30 Days', value: Duration.ThirtyDay }, | ||
| ], | ||
| }, | ||
| }), | ||
| ...getRecommendationDurationProperty(), | ||
| basis: Property.StaticDropdown({ |
There was a problem hiding this comment.
Swapping duration to a dynamic dropdown restricts what the UI presents, but it doesn’t prevent existing saved steps (or dynamic-mode values) from submitting ten-day for AWS + s3. If the intent is to make the action accept only 30 days for S3, please also enforce this in run (e.g., coerce duration to Duration.ThirtyDay when vendor/recommendationType indicate S3).
There was a problem hiding this comment.
Should we do that in run?
There was a problem hiding this comment.
i think its okay, we dont usually enforce if its the only thing people can select from options. but please check if it breaks existing workflows, so have one open thats set to any service thats not s3 like ec2 + 10 days, and if you reload with the code changes if it stays intact or not
There was a problem hiding this comment.
Looks fine to me, I tried EC2 + 10 & also 30 days. It stays intact.
ravikiranvm
changed the title
bigfluffycookie
left a comment
bigfluffycookie
left a comment
There was a problem hiding this comment.
just one comment
Fixes OPS-4202.
Additional Notes