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

ci: add makefile #800

Merged
merged 3 commits into from
Aug 24, 2022
Merged

ci: add makefile #800

merged 3 commits into from
Aug 24, 2022

Conversation

pgier
Copy link
Contributor

@pgier pgier commented Jun 30, 2022

Makefiles are commonly used for building golang projects.

  • add a Makefile with basic build, test, clean, etc.
  • simplify the Dockerfile by moving some scripts and conf files to directories
  • update README
  • Allows easy overriding of pulsar version and golang version used during testing

Motivation

Add a Makefile to make it easier to remember the commands for building and testing the project. For example:

  • make build instead of go build ./pulsar
  • make test instead of ./docker-ci.sh
  • make lint instead of golangci-lint run

@pgier pgier force-pushed the add-makefile branch 2 times, most recently from a04a45a to 1034dc9 Compare June 30, 2022 22:55
Copy link
Contributor

@zzzming zzzming left a comment

Choose a reason for hiding this comment

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

I appreciate these changes. I would recommend use of Makefile. But it comes down to project and community's choice. @merlimat @cckellogg @wolfstudy can help decide the direction.

Comment on lines +20 to +23
ARG PULSAR_IMAGE=apachepulsar/pulsar:latest
ARG GOLANG_IMAGE=golang:latest
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd better to keep the fixed version of base image. It is hard to trouble shoot problems if the latest points to a new version. That forces other PRs to deal with version problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The version is fixed in the Makefile, so CI builds would always use the version set there. I set these to default to latest so that they don't become out of date. I can add a comment to the Dockerfile saying that these versions should be set via the Makefile or CLI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment about setting the version during build.

Dockerfile Outdated Show resolved Hide resolved
README.md Outdated
@@ -24,9 +24,9 @@
[![LICENSE](https://img.shields.io/hexpm/l/pulsar.svg)](https://github.com/apache/pulsar-client-go/blob/master/LICENSE)
# Apache Pulsar Go Client Library

A Go client library for the [Apache Pulsar](https://pulsar.incubator.apache.org/) project.
A Go client library for [Apache Pulsar](https://pulsar.incubator.apache.org/).
Copy link
Member

Choose a reason for hiding this comment

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

the link should be updated, too :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

The link is an old incubator link. It should be updated without incubator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it's updated now.

@pgier pgier force-pushed the add-makefile branch 2 times, most recently from 10b6ad9 to 12ab1dc Compare August 12, 2022 18:52
- add a Makefile with basic build, test, clean, etc.
- simplify the Dockerfile by moving some scripts and conf files to directories
- update README
- allow easy setting of GOLANG version and PULSAR version for testing

Signed-off-by: Paul Gier <paul.gier@datastax.com>
Copy individual config files instead of directory when building the Dockerfile to catch any missing/incorrect files.

Signed-off-by: Paul Gier <paul.gier@datastax.com>
Signed-off-by: Paul Gier <paul.gier@datastax.com>
Copy link
Member

@nodece nodece left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your contribution @pgier!

@michaeljmarshall michaeljmarshall merged commit 2d5f6fc into apache:master Aug 24, 2022
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.

None yet

5 participants