Adding cfn-lint as optional parameter for SAM validate command by cdavidxu-hub · Pull Request #4444 · aws/aws-sam-cli

cdavidxu-hub · 2022-11-25T18:11:51Z

Why is this change necessary?

To provide linting validation to SAM cli and fulfill feature and customer requests

How does it address the issue?

Cfn-lint will be added as an optional parameter to SAM validate. When run, the linter will parse and provide detailed error messages and codes to the customer along with suggestions for potential fixes.

What side effects does this change have?

This should not pose any side-effects either to core functionality of SAM validate or to CI/CD processes as it remains an optional parameter.

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.

sriram-mv · 2022-11-29T21:40:30Z

 import boto3
 from botocore.exceptions import NoCredentialsError
 import click
+import cfnlint.core # type: ignore


The number of imports at the top level should be as small as possible, if we really need it we should move it within cli method. Also what is the reason behind the #type: ignore , I'd rather not have us skip any lint checks.

Have moved below to method below. It seems cfn-lint does not have a py.typed file/type hints and we likely will not be adding them before release. Is it alright to suppress for now?

sriram-mv · 2022-11-29T21:47:17Z

+    help="Run linting validation on template through cfn-lint. "
+     "For more information, see: https://github.com/aws-cloudformation/cfn-lint",
+    cls=ClickMutex,
+    incompatible_params=["config-env", "config-file", "profile", "region"]


why are these incompatible? you could very well have the template file be passed in through a configuration parameter within the 'validate' space such that it is default.validate.parameters, which has a template specified.

I see that cfn-lint has a way to pass in regions to its command line. what does that mapping mean here?

Does cfn-lint have any environment variables that it obeys? can they be used in conjunction through sam validate --lint ?

Hmmm, I see fair point - missed that. Would setting the template in the config-file also pass the template file path to the do_cli method? As for regions, I can include that as an option in the _lint method as well.

sriram-mv · 2022-11-29T21:49:36Z

+                with open(args.output_file, 'w', encoding='utf-8') as output_file:
+                    output_file.write(matches_output)
+            else:
+                print(matches_output)


we should use click's functionality for outputting information instead of using a direct print.

will look into it

sriram-mv · 2022-11-29T23:49:48Z

I also wonder if this should be its own command instead, i.e sam lint. Open Q

mndeveci · 2022-11-30T00:20:04Z

+
+    try:
+        if ctx.debug:
+            (args, filenames, formatter) = cfnlint.core.get_args_filenames([template, "--debug"])


Can we add details why we need special handling of --debug flag here?

We need a way to pass the --debug flag to cfn-lint since cfn-lint also takes in a `--debug- optional parameter as well. Do you mean you would want some comments in the code explaining why?

mndeveci · 2022-11-30T00:21:08Z

+        matches_output = formatter.print_matches(matches, rules, filenames)
+
+        if matches_output:
+            if args.output_file:


Where this output_file comes from and how users can set it?

This is an cfn-lint optional parameter which we cannot specify for sam validate so it will never output to this file. Will remove in next commit. Optionally we can have it always print to an output file but we would need to have that be explicitly stated to the customer

mndeveci · 2022-11-30T00:21:58Z

+        LOGGER.error(str(e))
+        return e.exit_code


Raise a UserException which will handle the error message and exit code, rather than manually returning the exit_code.

will look into it, sounds good

mndeveci · 2022-11-30T00:22:43Z


    return sam_template
+
+def _lint(ctx, template):


We are trying to add more typing into our repository, so can you please add typing for the parts that you have added?

will look into it and add

cdavidxu-hub · 2022-12-02T19:16:14Z

+        if matches_output:
+            click.secho(matches_output)
+
+    except cfnlint.core.InvalidRegionException as e:


Just to add some more context on why there are two region exceptions. Cfn-lint can catch invalid regions in commands like samdev —lint —region us-southeast-2 and throw a cfnlint.core.InvalidRegionException. However, if the customer forgets to specify a region and then uses another optional parameter, like samdev —lint —region —debug, the cfn-lint help page will be outputted as it won’t detect any arguments for —region. Cfn-lint uses argparse and is configured to display the help page upon errors which we don’t want exposed to customers right now. I’ve added the botocore.util.validate_region_name method to check the format of the region string which will catch cases when the customer forgets to specify a region and uses another parameter. Let me know if this approach is acceptable or if there is a better way to raise these exceptions.

However, if the customer forgets to specify a region and then uses another optional parameter, like samdev —lint —region —debug, the cfn-lint help page will be outputted as it won’t detect any arguments for —region

Curious, we are using cfn-lint as a library here, why does argparse matter?

The cfn-lint libraries make use of argparse so when specific errors occur the behaviour defaults to the argparse behaviour, like in this case with InvalidRegionExceptions.

praneetap · 2022-12-03T07:59:08Z

I also wonder if this should be its own command instead, i.e sam lint. Open Q

curious why you think that Sriram. Making it a new command would make discovery an issue, and sam validate is already plugged into sam deploy, which is a nice benefit here??

cdavidxu-hub · 2022-12-06T19:13:04Z

I also wonder if this should be its own command instead, i.e sam lint. Open Q

curious why you think that Sriram. Making it a new command would make discovery an issue, and sam validate is already plugged into sam deploy, which is a nice benefit here??

Sorry, missed these comments - I think in speaking with the design team, awareness was certainly an issue and since linting is a familiar term and form of validation we decided to keep it as an optional parameter for sam validate.

sriram-mv · 2022-12-06T23:05:49Z

@praneetap @cdavidxu-hub The thinking of sam lint was behind the fact we can make linting a first class citizen rather than a flag. I agree on the discoverability aspect though. sam validate --lint to me is unclear directly. Am I validating and linting at the same time? Am I just linting? In this case we are running cfn-lint alone if sam validate --lint is specified.

This goes against other commands, eg: sam build --use-container I'm still building (doing the same exact building logic) but within the container.

sriram-mv · 2022-12-06T23:07:48Z

+            ) from e
+
+        click.secho(
+            "{} can be transformed to a Cloudformation template. "


could we still add "is a valid SAM Template" back and then continue to add the rest? There could be tools that are scrapping our output.

I can see why that is a concern, however, the point of adding lint was that SAM validate was allowing false positives and outputting that a template was a valid SAM template when in fact it was not.

sriram-mv · 2022-12-06T23:10:29Z

+        if matches_output:
+            click.secho(matches_output)
+
+    except cfnlint.core.InvalidRegionException as e:


However, if the customer forgets to specify a region and then uses another optional parameter, like samdev —lint —region —debug, the cfn-lint help page will be outputted as it won’t detect any arguments for —region

Curious, we are using cfn-lint as a library here, why does argparse matter?

sriram-mv · 2022-12-13T00:25:43Z

+    cfn_lint_logger.propagate = False
+
+    try:
+        validate_region_name(ctx.region)


Lets move this entire check to within the cli validator for region

from samcli.commands.exceptions import RegionError def callback(ctx, param, value): state = ctx.ensure_object(Context) from botocore import exceptions, utils try: utils.validate_region_name(value) except exceptions.InvalidRegionError as ex: raise RegionError( message=f"Provided region: {value} doesn't match a supported format", wrapped_from=ex.__class__.__name__ ) state.region = value return value return click.option( "--region", expose_value=False, help="Set the AWS Region of the service (e.g. us-east-1).", callback=callback )(f)

at https://github.com/aws/aws-sam-cli/blob/develop/samcli/cli/options.py#L41

@cdavidxu-hub I think it was a misunderstanding that I didn't want to keep as it is, instead I was suggesting to move this validation to a more generic place or removing it completely. Let's continue with what @sriram-mv suggested above.

mndeveci · 2022-12-13T00:28:08Z

    --hash=sha256:fa6693661a4c91757f4412306191b6dc88c1703f780c8234035eac011922bc01 \
    --hash=sha256:fcd131dd944808b5bdb38e6f5b53013c5aa4f334c5cad0c72742f6eba4b73db0
    # via cryptography
+cfn-lint==0.72.2 \


Are there any dependencies which is dynamically imported? If so we might need to add them to hidden imports information for pyinstaller.

mndeveci · 2022-12-13T00:29:41Z

+    cfn_lint_logger.propagate = False
+
+    try:
+        validate_region_name(ctx.region)


@cdavidxu-hub I think it was a misunderstanding that I didn't want to keep as it is, instead I was suggesting to move this validation to a more generic place or removing it completely. Let's continue with what @sriram-mv suggested above.

mndeveci · 2022-12-13T23:12:54Z

        with self.assertRaises(UserException):
            _lint(ctx=ctx_lint_mock(debug=False, region="region"), template=template_path)

-    @patch("botocore.utils.validate_region_name")


Why we have removed this test, can we add it back with more generic solution that you have implemented?

sriram-mv · 2022-12-14T00:49:01Z

            result = runner.invoke(cli, ["local", "generate-event", "s3"])
            self.assertEqual(result.exit_code, 0)

+    def test_cli_with_no_region_arg_validate(self):


def test_cli_with_non_standard_format_region(self): mock_cfg = Mock() with patch("samcli.cli.main.GlobalConfig", mock_cfg): runner = CliRunner() for command in ["validate", "deploy"]: result = runner.invoke(cli, [command, "--region", "--non-standard-format"]) self.assertEqual(result.exit_code, 1) self.assertIn("Error: Provided region: --non-standard-format doesn't match a supported format", result.output)

You could do a assertRaises too.

Conversely if you wanted to test that region being set to a standard format went ok.

@patch("samcli.commands.validate.validate.do_cli") def test_cli_with_valid_region(self, mock_do_cli): mock_cfg = Mock() with patch("samcli.cli.main.GlobalConfig", mock_cfg): runner = CliRunner() result = runner.invoke(cli, ["validate", "--region", "us-west-2"]) self.assertEqual(result.exit_code, 0) self.assertTrue(mock_do_cli.called) self.assertEqual(mock_do_cli.call_count, 1)

sriram-mv · 2022-12-14T00:52:02Z

+            runner.invoke(cli, ["validate", "--region", "--debug"])
+            self.assertRaises(RegionError)
+
+    def test_cli_with_no_region_arg_deploy(self):


I don't quite get this test.

why does deploy use use-container False?

Hmm I took the parameters from a separate test, but ultimately I just want to test the region behaviour. Would it work with "deploy --region --debug"? If so, I will just remove the use-container parameter

mndeveci · 2022-12-14T01:17:17Z

+        mock_cfg = Mock()
+        with patch("samcli.cli.main.GlobalConfig", mock_cfg):
+            runner = CliRunner()
+            runner.invoke(cli, ["deploy", "use-container" "False", "--region", "--debug"])


It seems like both tests are using an empty region since another flag is provided right after --region? I would rather have;

A test with a valid region

A test with an invalid region

A test with empty region

sriram-mv · 2022-12-14T02:26:13Z

            result = runner.invoke(cli, ["local", "generate-event", "s3"])
            self.assertEqual(result.exit_code, 0)

+    def test_cli_with_no_region_argument(self):


this is not a no region argument test. its a non standard format test.

for a no region argument, it would look like this.

def test_cli_with_empty_region(self): mock_cfg = Mock() with patch("samcli.cli.main.GlobalConfig", mock_cfg): runner = CliRunner() for command in ["validate", "deploy"]: result = runner.invoke(cli, [command, "--region"]) self.assertEqual(result.exit_code, 2) self.assertIn("Error: Option '--region' requires an argument", result.output)

) * Adding cfn-lint optional parameter to SAM validate command * Cfn-lint optional parameter SAM validate make pr changes * Fix make pr and broken tests * Add Click error handling and allow config params * Add unit and integration test * fix formatting and region exceptions * Add integ unhappy path integ test and fix comments * Fix: remove local path string from test * generalize region validation and add license info * Fix licensing and dynamic installer * Added generalized region validation tests * Fix unit tests and change output string * Add valid region test, fix formatting * Add no region test Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com>

aws#4444)" This reverts commit 2fd533f.

* Revert "fix: `hooks` data imports for pyinstaller (#4491)" This reverts commit bea3bc0. * Revert "fix: Update expected message read validate lint integration test (#4488)" This reverts commit abd7c03. * Revert "Update lint helpand output message (#4489)" This reverts commit 3304955. * Revert "fix: `pyinstaller` binaries (#4486)" This reverts commit b8a939d. * Revert "fix: Fix validate command integration tests console output missmatch and update pyyaml version requirement (#4479)" This reverts commit ce7143c. * Revert "Adding cfn-lint as optional parameter for SAM validate command (#4444)" This reverts commit 2fd533f.

* Revert "fix: `hooks` data imports for pyinstaller (aws#4491)" This reverts commit bea3bc0. * Revert "fix: Update expected message read validate lint integration test (aws#4488)" This reverts commit abd7c03. * Revert "Update lint helpand output message (aws#4489)" This reverts commit 3304955. * Revert "fix: `pyinstaller` binaries (aws#4486)" This reverts commit b8a939d. * Revert "fix: Fix validate command integration tests console output missmatch and update pyyaml version requirement (aws#4479)" This reverts commit ce7143c. * Revert "Adding cfn-lint as optional parameter for SAM validate command (aws#4444)" This reverts commit 2fd533f.

cdavidxu-hub added 2 commits November 25, 2022 02:07

Adding cfn-lint optional parameter to SAM validate command

02c98dd

Cfn-lint optional parameter SAM validate make pr changes

e5de168

cdavidxu-hub requested a review from a team as a code owner November 25, 2022 18:11

cdavidxu-hub requested review from jfuss and mndeveci November 25, 2022 18:11

github-actions Bot added area/validate sam validate command pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Nov 25, 2022

cdavidxu-hub and others added 2 commits November 28, 2022 19:50

Fix make pr and broken tests

6543447

Merge branch 'aws:develop' into develop

4a3ec3f

sriram-mv suggested changes Nov 29, 2022

View reviewed changes

mndeveci suggested changes Nov 30, 2022

View reviewed changes

cdavidxu-hub and others added 2 commits December 1, 2022 16:48

Merge branch 'aws:develop' into develop

d0e6948

Add Click error handling and allow config params

949307d

cdavidxu-hub commented Dec 2, 2022

View reviewed changes

torresxb1 removed the stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. label Dec 3, 2022

cdavidxu-hub and others added 2 commits December 6, 2022 10:19

Merge branch 'aws:develop' into develop

aad36c6

Add unit and integration test

b2e4686

sriram-mv suggested changes Dec 6, 2022

View reviewed changes

cdavidxu-hub and others added 2 commits December 7, 2022 09:50

Merge branch 'aws:develop' into develop

d896752

fix formatting and region exceptions

9c57c5b

mndeveci suggested changes Dec 8, 2022

View reviewed changes

Comment thread requirements/base.txt

Comment thread samcli/commands/validate/validate.py Outdated

Comment thread samcli/commands/validate/validate.py Outdated

Comment thread samcli/commands/validate/validate.py Outdated

Comment thread tests/integration/validate/test_validate_command.py

cdavidxu-hub and others added 2 commits December 8, 2022 12:52

Merge branch 'aws:develop' into develop

32df182

Add integ unhappy path integ test and fix comments

ac52c74

sriram-mv suggested changes Dec 9, 2022

View reviewed changes

Comment thread samcli/commands/validate/validate.py Outdated

Merge branch 'aws:develop' into develop

dc04436

Fix: remove local path string from test

35dc28f

sriram-mv removed the request for review from jfuss December 9, 2022 21:20

aws-sam-cli-bot added the maintainer/need-followup label Dec 12, 2022

sriram-mv suggested changes Dec 13, 2022

View reviewed changes

mndeveci reviewed Dec 13, 2022

View reviewed changes

aws-sam-cli-bot removed the maintainer/need-followup label Dec 13, 2022

cdavidxu-hub and others added 3 commits December 13, 2022 11:15

Merge branch 'aws:develop' into develop

fd41d40

generalize region validation and add license info

9fbfa24

Fix licensing and dynamic installer

5430d5e

mndeveci reviewed Dec 13, 2022

View reviewed changes

mndeveci and others added 2 commits December 13, 2022 16:13

Merge branch 'develop' into develop

4b0f45f

Added generalized region validation tests

08872ca

sriram-mv suggested changes Dec 14, 2022

View reviewed changes

mndeveci reviewed Dec 14, 2022

View reviewed changes

cdavidxu-hub added 2 commits December 14, 2022 01:37

Fix unit tests and change output string

cb70eb9

Add valid region test, fix formatting

060dcd3

sriram-mv reviewed Dec 14, 2022

View reviewed changes

Add no region test

fa7e409

mndeveci approved these changes Dec 14, 2022

View reviewed changes

sriram-mv self-requested a review December 14, 2022 03:16

sriram-mv approved these changes Dec 14, 2022

View reviewed changes

sriram-mv enabled auto-merge (squash) December 14, 2022 03:16

sriram-mv merged commit 2fd533f into aws:develop Dec 14, 2022

mndeveci added a commit to mndeveci/aws-sam-cli that referenced this pull request Dec 16, 2022

Revert "Adding cfn-lint as optional parameter for SAM validate command (

f78d426

aws#4444)" This reverts commit 2fd533f.

Uh oh!

Conversation

cdavidxu-hub commented Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why is this change necessary?

How does it address the issue?

What side effects does this change have?

Mandatory Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sriram-mv commented Nov 29, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdavidxu-hub Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

praneetap commented Dec 3, 2022

Uh oh!

cdavidxu-hub commented Dec 6, 2022

Uh oh!

sriram-mv commented Dec 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdavidxu-hub commented Nov 25, 2022 •

edited

Loading

cdavidxu-hub Dec 2, 2022 •

edited

Loading