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

fix(deps): upgrade cfn-lint range #838 #839

Merged
merged 5 commits into from
Oct 31, 2024

Conversation

dhutchison
Copy link
Contributor

@dhutchison dhutchison commented Sep 25, 2024

Overview

Widens the dependency range for cfn-lint to include the latest 1.x releases for #838.

Testing/Steps taken to ensure quality

Assuming existing test suite would pick up any incompatibilities with the newer versions.

This has been tested by installing pinned versions of cfn-lint for 0.80.1, 1.3.1 and the latest 1.16.0, and running pytest each time. An incompatibility was found with the first non-pre-release 1.x version (1.3.0), as it seemed to be missing the method to actually run checks (I'm assuming that was a regression).

Although I did have trouble running these locally initially - I think I'm just missing something and possibly the contributor guide needs updated to include steps to ensure the local environment is configured correctly and tests run locally.

Testing Instructions

How to test this PR Start after checking out this branch (bulleted)

  • Assuming existing unit test suite would pick up a dependency version incompatibility, and that these tests are ran automatically for PRs

andrew-glenn edit: fixes #838

andrew-glenn
andrew-glenn previously approved these changes Sep 25, 2024
@andrew-glenn
Copy link
Collaborator

I don't see any reason the static analysis would fail on this. I'm OOTO today but will merge and cut a new release tomorrow.

Thank you very much for the PR! ❤️

tlindsay42
tlindsay42 previously approved these changes Sep 25, 2024
@tlindsay42
Copy link
Member

+1 thanks @dhutchison!

@dhutchison
Copy link
Contributor Author

Thanks that was super fast.... Yeah I saw those same errors on pre-commit, but not an area that I was remotely close to so had skipped over them. Might be worth trying running all the checks/tests yourself locally ahead of merge as something seems off with the pre-commit checks to me.

@dhutchison
Copy link
Contributor Author

Actually sorry, hold that. So the pylint failure prevents it getting as far as the local unit tests. I was seeing one failing due to a missing dependency, but adding that in manually exposes some test failures, so I'll take this back to draft.

Might be something I'm missing, but I had to manually install this into my venv to get tests to run

pip install ruamel.yaml

@dhutchison dhutchison marked this pull request as draft September 25, 2024 21:49
cfnlint.core no longer exposes REGION, but it has always been available through helpers
@dhutchison dhutchison dismissed stale reviews from tlindsay42 and andrew-glenn via b9b2314 September 25, 2024 22:18
@dhutchison
Copy link
Contributor Author

I've added a few changes to the pylint and pre-commit configurations to skip over the existing failures (assuming these are caused by changes in pylint as not pinned to an exact version).

Also increased the minimum cfn-lint version by a few minors to one that the tests pass on.

Still trying to work through tests on the newer cfn-lint. Getting a weird error that I can't see how is caused by how taskcat is using it

FAILED tests/test_cfn_lint.py::TestCfnLint::test_lint - cfnlint.exceptions.DuplicateRuleError: Rule already included: E1151
FAILED tests/test_cfn_lint.py::TestCfnLint::test_output_results - cfnlint.exceptions.DuplicateRuleError: Rule already included: E1151
FAILED tests/test_cfn_lint.py::TestCfnLint::test_passed - cfnlint.exceptions.DuplicateRuleError: Rule already included: E1151
FAILED tests/test_cli_module_lint.py::TestLintCli::test_lint - cfnlint.exceptions.DuplicateRuleError: Rule already included: E1151
FAILED tests/testing_module/test_cfn.py::TestCFNTest::test_run - cfnlint.exceptions.DuplicateRuleError: Rule already included: E1151

@dhutchison dhutchison force-pushed the task/update-cfn-lint branch from 26b45d7 to e2ac815 Compare October 3, 2024 22:14
@dhutchison
Copy link
Contributor Author

Well that was a bit of a journey through the history of cfn-lint... to maintain backwards compatibility with older versions I have included a few conditionals which check the version of cfn-lint installed. There quite likely is better ways to do this, but I wanted to avoid changing too much code.

I would appreciate if someone could double check the removal of the cfnlint.core.get_template_rules call from _cfn_lint.py - this does not appear to exist in the 1.x stream, and I also could not work out what it was actually validating from digging through the cfn-lint code.

This PR also makes a few changes to .pre-commit-config.yaml and .pylintrc to suppress issues which were appearing before my changes started.

@dhutchison dhutchison marked this pull request as ready for review October 3, 2024 22:19
@shadycuz
Copy link
Contributor

Thanks for this PR @dhutchison ;)

@andrew-glenn Any chance you can merge this? I can't run taskcat on python 3.12, because of its dependency on an old version of cfn-lint that does not support 3.12. See #841

@andrew-glenn
Copy link
Collaborator

andrew-glenn commented Oct 31, 2024

Yup. I'll sort this out in the morning. Expect a new release tomorrow.

(Yeah, I know I said that earlier in this PR. I set a reminder in slack this time)

@andrew-glenn andrew-glenn marked this pull request as draft October 31, 2024 14:03
@andrew-glenn andrew-glenn marked this pull request as ready for review October 31, 2024 14:03
@andrew-glenn
Copy link
Collaborator

/do-e2e-tests

Copy link

End to end test has been scheduled

Copy link

E2E tests in progress

Copy link

@aws-ia-automator-prod aws-ia-automator-prod bot left a comment

Choose a reason for hiding this comment

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

E2E tests completed successfully

@andrew-glenn
Copy link
Collaborator

That's all she wrote for this one. I want to release this with #840, FYI.

# In 0.x, it only returned the loaded configuration.
# https://github.com/aws-cloudformation/cfn-lint/blob/f006cb5d8c7056923f3f21b31c14edfeed3804b5/src/cfnlint/config.py#L730
#
# This causes issues for us as the get_rules method combines the supplied value with the default rule list,
Copy link
Collaborator

Choose a reason for hiding this comment

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

❤️

@andrew-glenn andrew-glenn merged commit 7a7cb22 into aws-ia:main Oct 31, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for cfn-lint 1.x
4 participants