Skip to content

Fix up vndr tooling#42634

Merged
tianon merged 1 commit into
moby:masterfrom
kevpar:fix-vndr-tooling
Jul 14, 2021
Merged

Fix up vndr tooling#42634
tianon merged 1 commit into
moby:masterfrom
kevpar:fix-vndr-tooling

Conversation

@kevpar

@kevpar kevpar commented Jul 13, 2021

Copy link
Copy Markdown
Contributor
  • Fix the error message in hack/validate/vendor to specify that
    hack/vendor.sh should be run instead of vndr.
  • Fix hack/vendor.sh to also match on Windows paths for the whitelist.
    This allows the script to be run on Windows via Git Bash.

Signed-off-by: Kevin Parsons [email protected]

- Fix the error message in hack/validate/vendor to specify that
  hack/vendor.sh should be run instead of vndr.
- Fix hack/vendor.sh to also match on Windows paths for the whitelist.
  This allows the script to be run on Windows via Git Bash.

Signed-off-by: Kevin Parsons <[email protected]>
@kevpar
kevpar requested a review from tianon as a code owner July 13, 2021 16:47
@kevpar

kevpar commented Jul 13, 2021

Copy link
Copy Markdown
Contributor Author

@thaJeztah FYI

@thaJeztah thaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Thanks! (I was trying to get some not-yet-contributors on my team to open a PR for changing the message) 😂

Comment thread hack/vendor.sh

if [ $# -eq 0 ] || [ "$1" != "archive/tar" ]; then
vndr -whitelist=^archive/tar "$@"
vndr -whitelist='^archive[/\\]tar' "$@"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Interesting; it's not matching on package name but on directory? (I assume this was failing on Windows?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it looks like it's using the whitelist in two places:

So sadly not a package path :(

@thaJeztah

Copy link
Copy Markdown
Member

@tianon ptal

@tianon tianon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

This allows the script to be run on Windows via Git Bash.

This warms my soul ❤️

@tianon
tianon merged commit 40502f4 into moby:master Jul 14, 2021
@thaJeztah thaJeztah added this to the 21.xx milestone Jul 20, 2021
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.

4 participants