Skip to content
This repository has been archived by the owner on Oct 22, 2021. It is now read-only.

feat: Add httplint #1196

Merged
merged 4 commits into from
Aug 22, 2020
Merged

feat: Add httplint #1196

merged 4 commits into from
Aug 22, 2020

Conversation

viccuad
Copy link
Member

@viccuad viccuad commented Aug 6, 2020

Description

Lint for SUSE URLs with http against the appropriate files.

Motivation and Context

Fixes: #1195

How Has This Been Tested?

$ make httplint                                                                
SUSE URLS should start with 'https':
deploy/helm/kubecf/requirements.lock:  repository: http://opensource.suse.com/eirini-release/
deploy/helm/kubecf/requirements.lock:  repository: http://opensource.suse.com/bits-service-release/
deploy/helm/kubecf/requirements.yaml:  repository: http://opensource.suse.com/eirini-release/
deploy/helm/kubecf/requirements.yaml:  repository: http://opensource.suse.com/bits-service-release/
make: *** [Makefile:28: httplint] Error 4

Fix the URLs, then:

$ make httplint
$ echo $?
0

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code has security implications.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@jandubois
Copy link
Member

Feedback from DIrk on 🚀 chat:

for generic linting I think we need to take a different approach, otherwise people get around the linter issue by deploying on suse.dev or something else
or use foobar.github.io 😉
my lesson learned from the last 10 years of dealing with linters is that whenever people have a chance to bypass the linter error without first having to understand the purpose and the intention of the linter check , they will do so

Copy link
Member

@viovanov viovanov left a comment

Choose a reason for hiding this comment

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

Can you please move this script to kubecf-tools?
Looks like something we can reuse in other projects.

@jandubois
Copy link
Member

As Dirk mentioned, hard-coding suse.com makes the pattern quite limited. Why should we not catch references to github.com or any other domain?

I've checked the current repo for all http:// URLs currently used:

$ git grep http:// | perl -pe 's#.*(http://[^" ):]+).*#$1#' | sort -u
http://127.0.0.1
http://json-schema.org/draft-07/schema
http://localhost
http://opensource.suse.com/bits-service-release/
http://opensource.suse.com/eirini-release/
http://registry.mirror.example
http://thinkrelevance.com/blog/2011/11/15/documenting-architecture-decisions
http://www.apache.org/licenses/
http://www.apache.org/licenses/LICENSE-2.0

If we exclude the docs directory, it reduces slightly:

$ git grep http:// -- ':!doc/**' | perl -pe 's#.*(http://[^" ):]+).*#$1#' | sort -u
http://127.0.0.1
http://json-schema.org/draft-07/schema
http://localhost
http://opensource.suse.com/bits-service-release/
http://opensource.suse.com/eirini-release/
http://www.apache.org/licenses/
http://www.apache.org/licenses/LICENSE-2.0

So we could hardcode these as exceptions:

$ git grep -P 'http://(?!127|json-schema|localhost|www.apache)' -- ':!doc/**' | perl -pe 's#.*(http://[^" ):]+).*#$1#' | sort -u
http://opensource.suse.com/bits-service-release/
http://opensource.suse.com/eirini-release/

These last 2 hits should then be removed by #1179.

So I think there are 2 issues still to be discussed:

  • Should we exclude the doc/ directory from checking?
  • How will we deal with the exclusion list if we move this script to kubecf-tools.git?

My suggestion would be to have a local file in the repo being linted, e.g. .http_exceptions with one line for each allowed domain, and then construct the regexp dynamically from that file. In that case I would also not exclude the docs directory, but just add each http:// URL from the docs to the exception list as well (only if they can't be converted to https://, of course).

@jandubois
Copy link
Member

jandubois commented Aug 7, 2020

This is what I'm suggesting:

$ cat .http_exceptions
127.0.0.1
json-schema.org/draft-07/schema
localhost
www.apache.org/licenses/
# from the doc/ tree; can they be converted to https:// ?
registry.mirror.example
thinkrelevance.com/blog/2011/11/15/documenting-architecture-decisions

$ cat ./scripts/httplint.sh
#!/usr/bin/env bash
source scripts/include/setup.sh

HTTP_EXCEPTIONS=.http_exceptions
if [ ! -f "${HTTP_EXCEPTIONS}" ]; then
    echo "httplint requires \"${HTTP_EXCEPTIONS}\" file"
    exit 1
fi

EXCEPTIONS=$(perl -e 'while(<>){chomp; push @_,quotemeta unless /^#/} print join("|", @_)' < "${HTTP_EXCEPTIONS}")
if [ "$(git grep -P "http://(?!${EXCEPTIONS})" | wc -l)" -gt 0 ]; then
    echo "URLS should not start with 'http://' (with excepted domains listed in ${HTTP_EXCEPTIONS})"
    git grep -P "http://(?!${EXCEPTIONS})"
    exit 1
fi

$ ./scripts/httplint.sh
URLS should not start with 'http://' (with excepted domains listed in .http_exceptions)
deploy/helm/kubecf/requirements.lock:  repository: http://opensource.suse.com/eirini-release/
deploy/helm/kubecf/requirements.lock:  repository: http://opensource.suse.com/bits-service-release/
deploy/helm/kubecf/requirements.yaml:  repository: http://opensource.suse.com/eirini-release/
deploy/helm/kubecf/requirements.yaml:  repository: http://opensource.suse.com/bits-service-release/

$ echo $?
1

$ perl -e 'while(<>){chomp; push @_,quotemeta unless /^#/} print join("|", @_)' < .http_exceptions
127\.0\.0\.1|json\-schema\.org\/draft\-07\/schema|localhost|www\.apache\.org\/licenses\/|www\.apache\.org\/licenses\/LICENSE\-2\.0|registry\.mirror\.example|thinkrelevance\.com\/blog\/2011\/11\/15\/documenting\-architecture\-decisions

It allows to have comments in .http_exceptions as well. The quotemeta function is used to escape dots in the URL so they are no longer wildcard character.

The nice thing about this approach is that each repo can have its own exceptions file.

Note that you need to disable history expansion if you want to use the negative lookahead assertion from the commandline, otherwise !$ is expanded by the shell: set +o histexpand.

Finally, I think the exit code should always be 1 in the failure case, and not the number of matches. The exit code is an 8 bit number, so exit 256 would be the same as exit 0.

@viovanov
Copy link
Member

viovanov commented Aug 7, 2020

+1 on a more generic solution

  • I don't think we should exclude docs - we expect download links and samples in there, and they should be https too
  • exception lists should live in each project imo (like a gitignore)

@jandubois
Copy link
Member

jandubois commented Aug 7, 2020

Excellent, then I think we can just put my suggestion into kubecf-tools.git and wire it up.

The only improvement that just came to my mind is that the script should not throw an error when .http_exceptions doesn't exist; it should just run with a built-in exception list that only includes localhost:

HTTP_EXCEPTIONS=.http_exceptions
if [ -f "${HTTP_EXCEPTIONS}" ]; then
    EXCEPTIONS=$(perl -e 'while(<>){chomp; push @_,quotemeta unless /^#/} print join("|", @_)' < "${HTTP_EXCEPTIONS}")
else
    EXCEPTIONS="127\.0\.0\.1|localhost"
fi

if [ "$(git grep -P "http://(?!${EXCEPTIONS})" | wc -l)" -gt 0 ]; then
    echo "URLS should not start with 'http://' (with excepted domains listed in ${HTTP_EXCEPTIONS})"
    git grep -P "http://(?!${EXCEPTIONS})"
    exit 1
fi

That way a repo doesn't need to create a .http_exceptions file if they have nothing to put in there.

@viccuad
Copy link
Member Author

viccuad commented Aug 16, 2020

Updated consuming cloudfoundry-incubator/kubecf-tools#4. Cheers!

@viccuad viccuad requested a review from viovanov August 16, 2020 21:21
jandubois
jandubois previously approved these changes Aug 21, 2020
Add .http_exceptions for kubecf.
Add `make httplint` target, and include it in `make lint`.
jandubois
jandubois previously approved these changes Aug 21, 2020
@cfcontainerizationbot cfcontainerizationbot added the pr-test-queue PRs with this label get tested label Aug 21, 2020
@github-actions github-actions bot removed the pr-test-queue PRs with this label get tested label Aug 21, 2020
jandubois
jandubois previously approved these changes Aug 21, 2020
@cfcontainerizationbot cfcontainerizationbot added the pr-test-queue PRs with this label get tested label Aug 21, 2020
@github-actions github-actions bot removed the pr-test-queue PRs with this label get tested label Aug 21, 2020
@cfcontainerizationbot cfcontainerizationbot added the pr-test-queue PRs with this label get tested label Aug 21, 2020
@github-actions github-actions bot removed the pr-test-queue PRs with this label get tested label Aug 21, 2020
@cfcontainerizationbot cfcontainerizationbot added the pr-test-queue PRs with this label get tested label Aug 22, 2020
@github-actions github-actions bot removed the pr-test-queue PRs with this label get tested label Aug 22, 2020
@viovanov viovanov merged commit 0b17c69 into master Aug 22, 2020
@viovanov viovanov deleted the add-httplint branch August 22, 2020 17:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SUSE URLs should use https://
4 participants