-
Notifications
You must be signed in to change notification settings - Fork 72
GT-60 OPS CLI with Arango Task #1003
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
Conversation
5200ca4 to
088f501
Compare
088f501 to
4016bce
Compare
4016bce to
a66a56a
Compare
| $(VBIN_LINUX_AMD64): $(SOURCES) dashboard/assets.go VERSION | ||
| @mkdir -p $(BINDIR)/$(RELEASE_MODE)/linux/amd64 | ||
| CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build ${GOBUILDARGS} --tags "$(RELEASE_MODE)" $(COMPILE_DEBUG_FLAGS) -installsuffix netgo -ldflags "-X $(REPOPATH)/pkg/version.version=$(VERSION) -X $(REPOPATH)/pkg/version.buildDate=$(BUILDTIME) -X $(REPOPATH)/pkg/version.build=$(COMMIT)" -o $(VBIN_LINUX_AMD64) ./ | ||
| CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build ${GOBUILDARGS} --tags "$(RELEASE_MODE)" $(COMPILE_DEBUG_FLAGS) -installsuffix netgo -ldflags "-X $(REPOPATH)/pkg/version.version=$(VERSION) -X $(REPOPATH)/pkg/version.buildDate=$(BUILDTIME) -X $(REPOPATH)/pkg/version.build=$(COMMIT)" -o $(VBIN_LINUX_AMD64) ./cmd/main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If both binaries are expected to have the same build params, it is possible to slightly simplify the code using $(foreach
Example in arangosync: https://github.com/arangodb/arangosync/blob/feature/arangosync-migration/Makefile#L157
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to implement this more refactor in the project files structure is required since the paths are different (i can not place all main files under the same paths due to the linter requirements)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jwierzbo I'm just worried we have another new binary is planned to be added and it would multiply the duplication even more. But I think it can be refactored in a separate PR 👍
nikita-vanyasin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with small suggestion
No description provided.