Skip to content
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

fsx: hour validation in LustreMaintenanceTime is not correct #30341

Closed
Labels
@aws-cdk/aws-fsx Related to Amazon FSx bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.

Comments

@mazyu36
Copy link
Contributor

mazyu36 commented May 26, 2024

Describe the bug

The validate method in the LustreMaintenanceTime checks hour property between 0 and 24.

if (!Number.isInteger(hour) || hour < 0 || hour > 24) {

But hour property should be between 0 and 23.

The following is a quotation from the CloudFormation docs.

HH is the zero-padded hour of the day (0-23), and MM is the zero-padded minute of the hour.

Expected Behavior

Should be validation error when hour property is set to 24.

Current Behavior

No validation error when hour property is set to 24.
But error occurs when deploy phase.

image

Reproduction Steps

import { Vpc } from 'aws-cdk-lib/aws-ec2';
import { App, RemovalPolicy, Stack } from 'aws-cdk-lib';
import { LustreDeploymentType, LustreFileSystem, LustreDataCompressionType, LustreMaintenanceTime, Weekday } from 'aws-cdk-lib/aws-fsx';

const app = new App();

const stack = new Stack(app, 'FsxLustre');

const vpc = new Vpc(stack, 'VPC', { restrictDefaultSecurityGroup: false });

const storageCapacity = 1200;
const lustreConfiguration = {
  deploymentType: LustreDeploymentType.SCRATCH_2,
  dataCompressionType: LustreDataCompressionType.LZ4,
  weeklyMaintenanceStartTime: new LustreMaintenanceTime({ day: Weekday.TUESDAY, hour: 24, minute: 0 }),
};
new LustreFileSystem(stack, 'FsxLustreFileSystem', {
  lustreConfiguration,
  storageCapacityGiB: storageCapacity,
  vpc,
  vpcSubnet: vpc.privateSubnets[0],
  removalPolicy: RemovalPolicy.DESTROY,
});

Possible Solution

Change validate method like this.

  private validate(hour: number, minute: number) {
    if (!Number.isInteger(hour) || hour < 0 || hour > 23) {  // change 24->23
      throw new Error('Maintenance time hour must be an integer between 0 and 23');  // change 24->23
    }

  // omit

Additional Information/Context

Even if you set the value to 24 for hour, it results in a deployment error, so in my understanding, the above change is not a breaking change.

CDK CLI Version

2.143.0

Framework Version

No response

Node.js Version

v20.6.0

OS

MacOS

Language

TypeScript

Language Version

No response

Other information

No response

@mazyu36 mazyu36 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 26, 2024
@github-actions github-actions bot added the @aws-cdk/aws-fsx Related to Amazon FSx label May 26, 2024
@mergify mergify bot closed this as completed in #30342 May 27, 2024
mergify bot pushed a commit that referenced this issue May 27, 2024
…LustreMaintenanceTime` class. (#30342)

### Issue # (if applicable)

Closes #30341 

### Reason for this change
The `hour` property in the `LustreMaintenanceTime` class should be between 0 and 23.

But no validation error occurs when it is set to 24.



### Description of changes
In the validate method, I changed it so that an error is thrown when the hour is greater than 23, instead of when the hour is greater than 24. 

This allows a validation error to occur when the hour is set to 24.



### Description of how you validated changes
Changed unit tests and add integ tests.


### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

atanaspam pushed a commit to atanaspam/aws-cdk that referenced this issue Jun 3, 2024
…LustreMaintenanceTime` class. (aws#30342)

### Issue # (if applicable)

Closes aws#30341 

### Reason for this change
The `hour` property in the `LustreMaintenanceTime` class should be between 0 and 23.

But no validation error occurs when it is set to 24.



### Description of changes
In the validate method, I changed it so that an error is thrown when the hour is greater than 23, instead of when the hour is greater than 24. 

This allows a validation error to occur when the hour is set to 24.



### Description of how you validated changes
Changed unit tests and add integ tests.


### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
vdahlberg pushed a commit to vdahlberg/aws-cdk that referenced this issue Jun 10, 2024
…LustreMaintenanceTime` class. (aws#30342)

### Issue # (if applicable)

Closes aws#30341 

### Reason for this change
The `hour` property in the `LustreMaintenanceTime` class should be between 0 and 23.

But no validation error occurs when it is set to 24.



### Description of changes
In the validate method, I changed it so that an error is thrown when the hour is greater than 23, instead of when the hour is greater than 24. 

This allows a validation error to occur when the hour is set to 24.



### Description of how you validated changes
Changed unit tests and add integ tests.


### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@aws-cdk-automation
Copy link
Collaborator

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.

@aws aws locked as resolved and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.