Skip to content

Conversation

shashax42
Copy link

@shashax42 shashax42 commented Mar 24, 2025

Which issue(s) does this change fix?

This PR adds a new feature and is not associated with a specific issue number.

Why is this change necessary?

The AWS SAM CLI's validate --lint command is useful for validating CloudFormation templates, but currently lacks the ability to apply rules specific to Serverless applications. Serverless applications have different characteristics compared to general CloudFormation templates, requiring additional validation rules tailored to their specific needs.

How does it address the issue?

This PR adds a --serverless-rules option to the sam validate --lint command, allowing users to leverage rules from the cfn-lint-serverless package. This enables users to perform additional validations specific to Serverless applications. When enabled, this option validates Serverless application best practices such as Lambda function memory size, timeout, log retention, API Gateway stage logging, and throttling settings.

What side effects does this change have?

  • This change does not affect existing functionality. The --serverless-rules option is only activated when explicitly specified.
  • If the cfn-lint-serverless package is not installed, users receive a clear error message prompting them to install the package.
  • When running in debug mode, additional information about Serverless Rules application and related settings is displayed.

Mandatory Checklist

PRs will only be reviewed after checklist is complete

  • Add input/output type hints to new functions/methods
  • Write design document if needed (Do I need to write a design document?)
  • Write/update unit tests
  • Write/update integration tests
  • Write/update functional tests if needed
  • make pr passes
  • make update-reproducible-reqs if dependencies were changed
  • Write documentation

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

@shashax42 shashax42 requested a review from a team as a code owner March 24, 2025 16:21
@github-actions github-actions bot added area/validate sam validate command area/schema JSON schema file pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Mar 24, 2025
@shashax42 shashax42 force-pushed the feature/serverless-rules-lint branch from 26e31e0 to b524fb7 Compare March 25, 2025 01:06
@vicheey vicheey added need-customer-response Waiting for customer response and removed stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Mar 26, 2025
@shashax42 shashax42 requested a review from vicheey April 4, 2025 08:51
@valerena
Copy link
Contributor

valerena commented Apr 7, 2025

Two comments.

  1. One thing to check here is if this works with SAM CLI installed without pip (like installed using the actual installer). I feel like that won't work (because SAM CLI uses their own Python environment, so it won't find cfn-lint-serverless installed). If that's the case (I haven't been able to test it yet to confirm), then this would be a functionality only for a particular set up and not for all SAM CLI customers. We can still launch it like that, but we need to be aware of that, for documentation purposes mainly.

  2. I feel like it would be a better approach to just pass "extra rules" to cfn-lint), and users will have to call it like:

sam validate --lint --extra-lint-rules "cfn_lint_serverless.rules"

(I don't know if that's the best name for the parameter, but that's the idea)

We would then keep this part of the code

linter_config["append_rules"] = extra_cfn_lint_config

but we won't do any special checks specifically for cfn-lint-serverless, and it would be more of a general purpose config. (Honestly, I don't know how many other extra "rules" exist for cfn-lint, but I imagine there are others that could be added to the validation).

- Mark existing --serverless-rules option as deprecated
- Add more flexible --extra-lint-rules option
- Implement support for comma-separated multiple rule modules
- Design to work regardless of installation environment
- Update documentation and schema accordingly
@shashax42 shashax42 force-pushed the feature/serverless-rules-lint branch from bd7404d to f75e51d Compare May 7, 2025 14:42
@shashax42
Copy link
Author

@valerena

Issue: Enhance --extra-lint-rules Feature in SAM CLI (Addressing Review Feedback)

Description

This PR enhances the --extra-lint-rules feature in SAM CLI, addressing all feedback from valerena.

Addressed Feedback

  1. Added multiple=True flag to --extra-lint-rules option to allow specifying multiple rule modules
  2. Completely removed all code and tests related to the never-officially-released --serverless-rules option
  3. Replaced debug print statements with proper logging module usage
  4. Changed all comments to English for consistency
  5. Thoroughly updated test cases to reflect the new implementation

Test Code Improvements

  • Removed obsolete test_serverless_rules_deprecated_with_extra_lint_rules test
  • Updated all test cases to handle extra_lint_rules as a tuple (supporting multiple=True)
  • Removed all references to serverless_rules from test parameters
  • Fixed assertions to match the new parameter structure
  • Changed Korean test comments to English

Implementation Details

  • Users can now use the --extra-lint-rules option multiple times or specify multiple modules separated by commas
  • Code is cleaner and more maintainable
  • Test code now correctly validates the new interface

Related PR

#7950

@shashax42 shashax42 requested a review from valerena August 10, 2025 16:01
@shashax42
Copy link
Author

@valerena
Will this PR be merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/schema JSON schema file area/validate sam validate command need-customer-response Waiting for customer response pr/external
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants