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

NETOBSERV-1498 Add shellcheck linter for bash and make help target #5

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

msherif1234
Copy link
Contributor

Add shellcheck bash scripts linter and fix linter error
Add make help target

@msherif1234 msherif1234 requested a review from jpinsonneau March 12, 2024 21:14
$(OCI_BIN) build -t network-observability-cli .

.PHONY: lint
lint: prereqs ## Lint code
@echo "### Linting code"
golangci-lint run ./...
@echo "### Run shellcheck against bash scripts"
find . -name '*.sh' | xargs shellcheck
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to install shellcheck automatically in the prereqs target ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no this a pkg u need to install on ur host and it depends on the os

Copy link
Contributor

Choose a reason for hiding this comment

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

the article above suggest:

  docker pull koalaman/shellcheck:latest
  docker run -v "$PWD:/mnt" koalaman/shellcheck "$1"

The advantage of this is that shellcheck will not be a requirement for devs / CI

Else we need to add that in the README

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated readme its not worth spinning up docker atm for single pkg dependency

Comment on lines +48 to +54
.PHONY: help
help: ## Display this help.
@awk 'BEGIN {FS = ":.*##"; printf "\nUsage:\n make \033[36m<target>\033[0m\n"} /^[a-zA-Z_0-9-]+:.*?##/ { printf " \033[36m%-15s\033[0m %s\n", $$1, $$2 } /^##@/ { printf "\n\033[1m%s\033[0m\n", substr($$0, 5) } ' $(MAKEFILE_LIST)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Comment on lines +1 to +3
#!/usr/bin/env bash
set -eux

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Don't spend too much effort on the scripts, I have pending PRs that changes everything in these:
#3
#4

We should consider merging them before fixing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok since this repo has bash dependency I thought having lint for bash will be add good value

@jpinsonneau jpinsonneau changed the title Add shellcheck linter for bash and make help target NETOBSERV-1498 Add shellcheck linter for bash and make help target Mar 13, 2024
Copy link
Contributor

@jpinsonneau jpinsonneau left a comment

Choose a reason for hiding this comment

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

LGTM, feel free to merge @msherif1234 😸

@msherif1234
Copy link
Contributor Author

/approve

@msherif1234 msherif1234 merged commit fa4dc73 into netobserv:main Mar 14, 2024
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.

2 participants