pingtimeout commented on code in PR #171: URL: https://github.com/apache/polaris-tools/pull/171#discussion_r2829289429
########## console/Makefile: ########## @@ -1,37 +1,91 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# Configures the shell for recipes to use bash, enabling bash commands and ensuring +# that recipes exit on any command failure (including within pipes). +SHELL = /usr/bin/env bash -o pipefail +.SHELLFLAGS = -ec + +## Variables DOCKER ?= docker DOCKER_IMAGE_NAME ?= apache/polaris-console DOCKER_IMAGE_TAG ?= latest -.PHONY: build-docker -build-docker: - @$(DOCKER) build -f docker/Dockerfile -t $(DOCKER_IMAGE_NAME):$(DOCKER_IMAGE_TAG) . +## Version information +BUILD_VERSION := $(shell grep version package.json | sed 's/.*"version": "\(.*\)".*/\1/') +GIT_COMMIT := $(shell git rev-parse HEAD) +##@ General -.PHONY: install -install: - npm install +.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%-40s\033[0m %s\n", $$1, $$2 } /^##@/ { printf "\n\033[1m%s\033[0m\n", substr($$0, 5) } ' $(MAKEFILE_LIST) -.PHONY: lint -lint: format-check - npm run lint +.PHONY: version +version: ## Display version information + @echo "Build version: ${BUILD_VERSION}" + @echo "Git commit: ${GIT_COMMIT}" -.PHONY: format-fix -format-fix: - npm run format +##@ Polaris Console -.PHONE: lint-fix -lint-fix: format-fix - npm run lint -- --fix +.PHONY: build +build: lint ## Lint the console project + @echo "--- Building console project---" + @npm run build + @echo "--- Console project built ---" + +.PHONY: build-docker +build-docker: install ## Build docker image for console project + @echo "--- Building docker image for console project---" + @$(DOCKER) build -f docker/Dockerfile -t $(DOCKER_IMAGE_NAME):$(DOCKER_IMAGE_TAG) . + @echo "--- Docker image for console project built ---" + +.PHONY: dev +dev: install ## Run the console project in development mode + @echo "--- Running console project in development mode ---" + @npm run dev + @echo "--- Console project in development mode completed ---" .PHONY: format-check -format-check: - npm run format:check +format-check: install ## Fix formatting in the console project + @echo "--- Checking formatting in the console project ---" + @npm run format:check + @echo "--- Formatting in the console project checked ---" -.PHONY: build -build: lint - npm run build +.PHONY: format-fix +format-fix: install ## Fix linting issues in the console project + @echo "--- Fixing formatting in the console project ---" + @npm run format + @echo "--- Formatting in the console project fixed ---" -.PHONY: dev -dev: - npm run dev \ No newline at end of file +.PHONY: install +install: ## Install dependencies for console project + @echo "--- Install dependencies for console project ---" + @npm install + @echo "--- Dependencies for console project completed ---" + +.PHONY: lint +lint: format-check ## Lint the console project + @echo "--- Linting the console project ---" + @npm run lint + @echo "--- Console project linted ---" + +.PHONE: lint-fix Review Comment: ```suggestion .PHONY: lint-fix ``` ########## console/Makefile: ########## @@ -1,37 +1,91 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# Configures the shell for recipes to use bash, enabling bash commands and ensuring +# that recipes exit on any command failure (including within pipes). +SHELL = /usr/bin/env bash -o pipefail +.SHELLFLAGS = -ec + +## Variables DOCKER ?= docker DOCKER_IMAGE_NAME ?= apache/polaris-console DOCKER_IMAGE_TAG ?= latest -.PHONY: build-docker -build-docker: - @$(DOCKER) build -f docker/Dockerfile -t $(DOCKER_IMAGE_NAME):$(DOCKER_IMAGE_TAG) . +## Version information +BUILD_VERSION := $(shell grep version package.json | sed 's/.*"version": "\(.*\)".*/\1/') +GIT_COMMIT := $(shell git rev-parse HEAD) +##@ General -.PHONY: install -install: - npm install +.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%-40s\033[0m %s\n", $$1, $$2 } /^##@/ { printf "\n\033[1m%s\033[0m\n", substr($$0, 5) } ' $(MAKEFILE_LIST) -.PHONY: lint -lint: format-check - npm run lint +.PHONY: version +version: ## Display version information + @echo "Build version: ${BUILD_VERSION}" + @echo "Git commit: ${GIT_COMMIT}" -.PHONY: format-fix -format-fix: - npm run format +##@ Polaris Console -.PHONE: lint-fix -lint-fix: format-fix - npm run lint -- --fix +.PHONY: build +build: lint ## Lint the console project Review Comment: nit: the description says "Lint" but the target is build? ########## console/Makefile: ########## @@ -1,37 +1,91 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# Configures the shell for recipes to use bash, enabling bash commands and ensuring +# that recipes exit on any command failure (including within pipes). +SHELL = /usr/bin/env bash -o pipefail +.SHELLFLAGS = -ec + +## Variables DOCKER ?= docker DOCKER_IMAGE_NAME ?= apache/polaris-console DOCKER_IMAGE_TAG ?= latest -.PHONY: build-docker -build-docker: - @$(DOCKER) build -f docker/Dockerfile -t $(DOCKER_IMAGE_NAME):$(DOCKER_IMAGE_TAG) . +## Version information +BUILD_VERSION := $(shell grep version package.json | sed 's/.*"version": "\(.*\)".*/\1/') +GIT_COMMIT := $(shell git rev-parse HEAD) +##@ General -.PHONY: install -install: - npm install +.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%-40s\033[0m %s\n", $$1, $$2 } /^##@/ { printf "\n\033[1m%s\033[0m\n", substr($$0, 5) } ' $(MAKEFILE_LIST) -.PHONY: lint -lint: format-check - npm run lint +.PHONY: version +version: ## Display version information + @echo "Build version: ${BUILD_VERSION}" + @echo "Git commit: ${GIT_COMMIT}" -.PHONY: format-fix -format-fix: - npm run format +##@ Polaris Console -.PHONE: lint-fix -lint-fix: format-fix - npm run lint -- --fix +.PHONY: build +build: lint ## Lint the console project + @echo "--- Building console project---" + @npm run build + @echo "--- Console project built ---" + +.PHONY: build-docker +build-docker: install ## Build docker image for console project + @echo "--- Building docker image for console project---" + @$(DOCKER) build -f docker/Dockerfile -t $(DOCKER_IMAGE_NAME):$(DOCKER_IMAGE_TAG) . + @echo "--- Docker image for console project built ---" + +.PHONY: dev +dev: install ## Run the console project in development mode + @echo "--- Running console project in development mode ---" + @npm run dev + @echo "--- Console project in development mode completed ---" .PHONY: format-check -format-check: - npm run format:check +format-check: install ## Fix formatting in the console project Review Comment: The comment says it fixes format but the task is `format:check`. Does that task automatically fix any error? Also, this line introduces a dependency between `format-check` and `install`. This means that anytime somebody runs `make build`, `make lint` or `make format-check`, a `npm install` command will be executed. Is this intended? ########## console/Makefile: ########## @@ -1,37 +1,91 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# Configures the shell for recipes to use bash, enabling bash commands and ensuring +# that recipes exit on any command failure (including within pipes). +SHELL = /usr/bin/env bash -o pipefail +.SHELLFLAGS = -ec + +## Variables DOCKER ?= docker DOCKER_IMAGE_NAME ?= apache/polaris-console DOCKER_IMAGE_TAG ?= latest -.PHONY: build-docker -build-docker: - @$(DOCKER) build -f docker/Dockerfile -t $(DOCKER_IMAGE_NAME):$(DOCKER_IMAGE_TAG) . +## Version information +BUILD_VERSION := $(shell grep version package.json | sed 's/.*"version": "\(.*\)".*/\1/') +GIT_COMMIT := $(shell git rev-parse HEAD) +##@ General -.PHONY: install -install: - npm install +.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%-40s\033[0m %s\n", $$1, $$2 } /^##@/ { printf "\n\033[1m%s\033[0m\n", substr($$0, 5) } ' $(MAKEFILE_LIST) -.PHONY: lint -lint: format-check - npm run lint +.PHONY: version +version: ## Display version information + @echo "Build version: ${BUILD_VERSION}" + @echo "Git commit: ${GIT_COMMIT}" -.PHONY: format-fix -format-fix: - npm run format +##@ Polaris Console -.PHONE: lint-fix -lint-fix: format-fix - npm run lint -- --fix +.PHONY: build +build: lint ## Lint the console project + @echo "--- Building console project---" + @npm run build + @echo "--- Console project built ---" + +.PHONY: build-docker +build-docker: install ## Build docker image for console project + @echo "--- Building docker image for console project---" + @$(DOCKER) build -f docker/Dockerfile -t $(DOCKER_IMAGE_NAME):$(DOCKER_IMAGE_TAG) . + @echo "--- Docker image for console project built ---" + +.PHONY: dev +dev: install ## Run the console project in development mode + @echo "--- Running console project in development mode ---" + @npm run dev + @echo "--- Console project in development mode completed ---" .PHONY: format-check -format-check: - npm run format:check +format-check: install ## Fix formatting in the console project + @echo "--- Checking formatting in the console project ---" + @npm run format:check + @echo "--- Formatting in the console project checked ---" -.PHONY: build -build: lint - npm run build +.PHONY: format-fix +format-fix: install ## Fix linting issues in the console project + @echo "--- Fixing formatting in the console project ---" + @npm run format + @echo "--- Formatting in the console project fixed ---" -.PHONY: dev -dev: - npm run dev \ No newline at end of file +.PHONY: install +install: ## Install dependencies for console project + @echo "--- Install dependencies for console project ---" + @npm install + @echo "--- Dependencies for console project completed ---" + +.PHONY: lint +lint: format-check ## Lint the console project + @echo "--- Linting the console project ---" + @npm run lint + @echo "--- Console project linted ---" + +.PHONE: lint-fix +lint-fix: format-fix ## Check formatting in the console project Review Comment: The comment says `## Check formatting in the console project` but this time it is about actually fixing linting issues? It seems that those comments are not in the right place, doesn't it? ########## benchmarks/Makefile: ########## @@ -1,49 +1,87 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at # -# Licensed to the Apache Software Foundation (ASF) under one -# or more contributor license agreements. See the NOTICE file -# distributed with this work for additional information -# regarding copyright ownership. The ASF licenses this file -# to you under the Apache License, Version 2.0 (the -# "License"); you may not use this file except in compliance -# with the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, -# software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -# KIND, either express or implied. See the License for the -# specific language governing permissions and limitations -# under the License. +# http://www.apache.org/licenses/LICENSE-2.0 # +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Configures the shell for recipes to use bash, enabling bash commands and ensuring +# that recipes exit on any command failure (including within pipes). +SHELL = /usr/bin/env bash -o pipefail +.SHELLFLAGS = -ec + +## Variables +APPLICATION_CONF_PATH ?= application.conf Review Comment: Is this intended to allow people to pass an absolute path to any `application.conf` file? AFAICT, any relative path would need to be determined from the benchmarks/ directory, right? ########## benchmarks/Makefile: ########## @@ -1,49 +1,87 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at # -# Licensed to the Apache Software Foundation (ASF) under one -# or more contributor license agreements. See the NOTICE file -# distributed with this work for additional information -# regarding copyright ownership. The ASF licenses this file -# to you under the Apache License, Version 2.0 (the -# "License"); you may not use this file except in compliance -# with the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, -# software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -# KIND, either express or implied. See the License for the -# specific language governing permissions and limitations -# under the License. +# http://www.apache.org/licenses/LICENSE-2.0 # +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Configures the shell for recipes to use bash, enabling bash commands and ensuring +# that recipes exit on any command failure (including within pipes). +SHELL = /usr/bin/env bash -o pipefail +.SHELLFLAGS = -ec + +## Variables +APPLICATION_CONF_PATH ?= application.conf + +## Version information +GIT_COMMIT := $(shell git rev-parse HEAD) + +##@ General + +.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%-40s\033[0m %s\n", $$1, $$2 } /^##@/ { printf "\n\033[1m%s\033[0m\n", substr($$0, 5) } ' $(MAKEFILE_LIST) + +.PHONY: version +version: ## Display version information + @echo "Git commit: ${GIT_COMMIT}" + +##@ Polaris Benchmark + +.PHONY: benchmarks-pre-requisite +benchmarks-pre-requisite: + @if [ ! -f "${APPLICATION_CONF_PATH}" ]; then \ + echo "ERROR: ${APPLICATION_CONF_PATH} is missing"; \ + exit 1; \ + fi + +.PHONY: create-commits-simulation +create-commits-simulation: benchmarks-pre-requisite ## Run create commits simulation + @echo "--- Running create commits simulation ---" Review Comment: ```suggestion @echo "--- Running create commits simulation ---" ``` nit: there seems to be some trailing whitespaces in a couple of places in this PR, could you ensure those are removed? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
