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

Add noqa and disable .ansible-lint global exclusions #6410

Merged
merged 1 commit into from
Jul 27, 2020

Conversation

Miouge1
Copy link
Contributor

@Miouge1 Miouge1 commented Jul 16, 2020

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Remove global ansible-lint exceptions, and replace them with local # noqa.

Additional notes:
Fixing all the instances of a given rule in one PR is difficult since it occurs in many roles (CNI, container engines, etc...). I suggest to drop the global exception add a bunch of # noqa to allow things to be fixed 1 task at a time rather than all or nothing.

I know it's not ideal, to have so many # noqa, but global exception is maybe worst?

/cc @MarkusTeufelberger

@k8s-ci-robot
Copy link
Contributor

@Miouge1: GitHub didn't allow me to request PR reviews from the following users: MarkusTeufelberger.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Remove global ansible-lint exceptions, and replace them with local # noqa.

Additional notes:
Fixing all the instances of a given rule in one PR is difficult since it occurs in many roles (CNI, container engines, etc...). I suggest to drop the global exception add a bunch of # noqa to allow things to be fixed 1 task at a time rather than all or nothing.

I know it's not ideal, to have so many # noqa, but global exception is maybe worst?

/cc @MarkusTeufelberger

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 16, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miouge1

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 16, 2020
@Miouge1 Miouge1 force-pushed the noqa-all-the-things branch 3 times, most recently from 58718a8 to cef8093 Compare July 16, 2020 14:50
@MarkusTeufelberger
Copy link
Contributor

As you see there are only <170 total linter errors for 7 rules, so I would say it is definitely feasible to fix these, especially as some are definitely easier to fix than others.

I would at least add something unique that can be queried since now it is not clear if some ansible-lint rule was violated willingly (e.g. because something needs to be done in a certain way) or just silenced in here to prevent new violations.

This just pushes the work down the road, there is no way around to fixing these eventually and once you enable e.g. 305 and 306 for new contributions you'll have no good examples to point to and will just force this work on potentially less experienced contributors how to write proper tasks that run on all supported (or rather: all tested...) platforms. It would be easier to test some idiom and apply this repository-wide rather than wait until someone sneakily deactivates this in a PR out of frustration again (see #5189 and #5230).

@k8s-ci-robot k8s-ci-robot removed the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 16, 2020
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jul 16, 2020
@Miouge1 Miouge1 force-pushed the noqa-all-the-things branch from 801897d to 6415e57 Compare July 16, 2020 15:33
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 16, 2020
@Miouge1 Miouge1 force-pushed the noqa-all-the-things branch from 6415e57 to a3dfd2a Compare July 17, 2020 06:56
@Miouge1
Copy link
Contributor Author

Miouge1 commented Jul 17, 2020

Thank you for the feedback @MarkusTeufelberger

I've added a comment in .ansible-lint to be more explicit: intentional noqa should have comment, otherwise I think it is safe to assume that they need to be looked into. It is possible to search for unintentional instances with noqa \d+$.

To be honest a I used a small Python script to inject the # noqa comments based on the output of ansible-lint.

This has been pretty helpful to search the codebase (my editor doesn't have support for ansible-lint rules highlighting, but searching for # noqa has been a game changer for me).

In this PR I've added commits to fix E305 and E306 (and a couple of E301). Using this, I've also created another PR for E404 (#6417).

With this work I also identified some playbooks to delete (see #6418).

At this stage, I do not intend to fix ansible-lint errors in contrib/, I think it is fair to focus on the core codebase rather than contrib folder.

As it stands in 94394f7 there are mostly E301, E303 and E503 left which I would prefer to address to follow up PRs to reduce the risk of merge conflicts.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2020
@Miouge1 Miouge1 force-pushed the noqa-all-the-things branch from 865758c to 6b177de Compare July 27, 2020 06:47
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 27, 2020
@Miouge1 Miouge1 force-pushed the noqa-all-the-things branch 2 times, most recently from 8d8c32f to f7ad95d Compare July 27, 2020 07:13
@floryut
Copy link
Member

floryut commented Jul 27, 2020

Looks good, CI is happy and lot of works to be done (love the branch name btw)
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 27, 2020
@Miouge1
Copy link
Contributor Author

Miouge1 commented Jul 27, 2020

I did a rebase and resolved merge conflicts. Some of the fixes will be split into a subsequent PR to avoid more conflicts.

@Miouge1 Miouge1 changed the title [WIP] Add noqa and disable .ansible-lint global exclusions Add noqa and disable .ansible-lint global exclusions Jul 27, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 27, 2020
@k8s-ci-robot k8s-ci-robot merged commit e70f27d into kubernetes-sigs:master Jul 27, 2020
erulabs added a commit to kubesail/kubespray that referenced this pull request Jul 29, 2020
* 'master' of https://github.com/kubernetes-sigs/kubespray:
  Documentation for Ingress (kubernetes-sigs#6378)
  Fix ansible-lint E301 for commands fetching data (kubernetes-sigs#6465)
  Fix shellcheck url (kubernetes-sigs#6462)
  Fix ansible-lint E305 (kubernetes-sigs#6459)
  Fix ansible-lint E404 (kubernetes-sigs#6417)
  Update README.md and openstack.md (kubernetes-sigs#6455)
  Add noqa and disable .ansible-lint global exclusions (kubernetes-sigs#6410)
  Move healthz check to secure ports (kubernetes-sigs#6446)
  Update multus version & crio conf (kubernetes-sigs#6444)
  Fix remove etcd broken with etcdctl_api 3 (kubernetes-sigs#6448)
  update cinder csi manifests (kubernetes-sigs#6434)
  Update docker package to 19.03.12 (kubernetes-sigs#6439)
  * add proxy_env definition to remove_node.yml resolving kubernetes-sigs#6430 (kubernetes-sigs#6431)
LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants