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

Adds a functional Makefile #96

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

brianbruggeman
Copy link

Adds a simple Makefile to standardize development

Addresses #95.

Adds a simple makefile to standardize development
@jonas32
Copy link
Contributor

jonas32 commented Jan 30, 2021

Looks fine.
I just found one little point i would try to avoid.
You hardcoded the ports into the config.
What do you think about making them as env vars with a default value?

In addition, do you see any way to use the normal not docker release of the server? Maybe building the server from a custom dockerfile.
While integrating Expressions, i figured out that the docker Image is a few days behind sometimes. Im not sure if this is a big deal but that made me spend some extra hours back then.

@brianbruggeman
Copy link
Author

I had actually thought about adding a port variable initially, but then rejected it because:

  1. I think it would be good to test against 3 ports rather than 1.
  2. If I wanted to add three ports, I think I'd need 3 port environment variables
  3. I think AEROSPIKE_HOSTS would probably need to reflect the choice of port environment variables
  4. The ports would really only be for the local system because the container would always use the default of 300x

At that point, I wondered if there was really much sense in adding a port variable.

That said I do want to make another update to the Makefile, so if you think there's enough of a good reason despite the above 4, I'm not opposed to adding it.

@jonas32
Copy link
Contributor

jonas32 commented Jan 30, 2021

No you are right. I didnt think about just overwriting the whole variable. Thats perfectly fine.

The makefile will now read a dotenv file if present
Copy link
Contributor

@jhecking jhecking left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! I think adding a Makefile with standarized commands to build, test, release the client is a great idea.

Makefile Outdated Show resolved Hide resolved
@brianbruggeman
Copy link
Author

@jonas32

I took your thoughts and made several port environment variables using: https://www.aerospike.com/docs/operations/configure/network/

At this point you can now override where they are resolved locally.

* Makes env command available in the help menu
* Removes help from the menu
* Adds env to run when a release command is run
* Reduces menu width
Each feature should be tested individually to determine if the code
works with and without the feature running.
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

3 participants