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

change: add file: Lintable argument to <rule_class>.match{,task} methods #1535

Conversation

ssato
Copy link
Contributor

@ssato ssato commented Apr 18, 2021

Add file: Lintable argument to match and matchtask methods of the rule
classes because there are cases that it is necessary to do some analyses
depends on the context, that is, file: Lintable, such like the followings.

  • want to restrict tasks in 'main.yml' use include_tasks only
  • want to restrict the level of inclusions with include_tasks to 2
  • restrict keywords can be used depends on file path or name
  • inhibit some keywords are used in the line depends on file name

Signed-Off-By: Satoru SATOH satoru.satoh@gmail.com

@ssbarnea
Copy link
Member

@ssato I do agree that Lintable should be exposed, at least for the matchtask.

For match(), it gets quite complicated and my plans were to deprecate and remove it. I personally see no good reason why a rule should look at a text line, especially as we are no longer doing YAML validations ourselves. IMHO, all the checks should be done using the already loaded data, not the source file.

Example:

myvar: >
  foo
  bar

The construct above represents the same thing as myvar: foo bar and our rules should focus on these.

The really tricky issue is that both of these changes will break all custom rules people may have, so we would need to do them in v6.

How about a compromise for matchtask that should avoid breaking users: add the Lintable argument as optional (second arg). I can see how a change like that would work, as I would not have to worry about undesired side-effects.

@ssbarnea ssbarnea self-requested a review April 19, 2021 08:27
@ssato
Copy link
Contributor Author

ssato commented Apr 20, 2021

@ssbarnea Thanks a lot for your feedbacks!

@ssato I do agree that Lintable should be exposed, at least for the matchtask.

For match(), it gets quite complicated and my plans were to deprecate and remove it. I personally see no good reason why a rule should look at a text line, especially as we are no longer doing YAML validations ourselves. IMHO, all the checks should be done using the already loaded data, not the source file.

Absolutely right. I removed that part modify .match() from the change to simplify it.

The really tricky issue is that both of these changes will break all custom rules people may have, so we would need to do them in v6.

How about a compromise for matchtask that should avoid breaking users: add the Lintable argument as optional (second arg). I can see how a change like that would work, as I would not have to worry about undesired side-effects.

Also I fully agree with you. I updated to allow calling matchtask() w/o the file: Lintable argument by setting it to None by default.

I pushed another commit brings these updates to make these changes easier to understand.
Could you please review it again? If those are OK, I'll squash that commit to the previous commit and 'git push -f' again.

@ssato ssato force-pushed the fix/backward-compatibilities-of-rule-match-methods branch from 9076ebf to 960687f Compare April 20, 2021 05:17
@@ -114,7 +114,7 @@ def matchtasks(self, file: Lintable) -> List[MatchError]:

if 'action' not in task:
continue
result = self.matchtask(task)
result = self.matchtask(file, task)
Copy link
Member

Choose a reason for hiding this comment

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

Clearly these calls use incorrect order or arguments. I hope they fail the linting as I do not want to see others doing the same mistake.

@ssbarnea ssbarnea self-requested a review April 20, 2021 06:03
Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

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

I did spot one error you need to fix, but in general it looks ok.

Run the "tox -e lint,py" locally and once all CI jobs are fixed it should be ready to merge.

Add 'file: Lintable' argument to matchtask method of the rule classes
which is None by default, because there are cases that it is necessary
to do some analyses depends on the context, that is, file: Lintable,
like the followings.

- want to restrict tasks in 'main.yml' use include_tasks only
- want to restrict the level of inclusions with include_tasks to 2

Signed-Off-By: Satoru SATOH <satoru.satoh@gmail.com>
@ssato ssato force-pushed the fix/backward-compatibilities-of-rule-match-methods branch from 960687f to 98e8e06 Compare April 20, 2021 09:52
@ssato
Copy link
Contributor Author

ssato commented Apr 20, 2021

I did spot one error you need to fix, but in general it looks ok.

Run the "tox -e lint,py" locally and once all CI jobs are fixed it should be ready to merge.

Thanks for your quick feedback!

I fixed all of the issues AFAIK, squashed commits and pushed with -f for merge.
At least 'tox -e lint,py39-devel,py39-ansible29,py36-ansible29' are passed locally.

@ssbarnea ssbarnea self-requested a review April 20, 2021 10:32
@ssbarnea ssbarnea merged commit eab3e1b into ansible:master Apr 20, 2021
@ssato ssato deleted the fix/backward-compatibilities-of-rule-match-methods branch May 2, 2021 02:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants