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

[Tooling] Create build makefile #390

Merged
merged 11 commits into from
Dec 22, 2022
Merged

[Tooling] Create build makefile #390

merged 11 commits into from
Dec 22, 2022

Conversation

Gustavobelfort
Copy link
Contributor

@Gustavobelfort Gustavobelfort commented Dec 10, 2022

Description

This pr is responsible for both implementing targets to build the pocket binaries defined in the app/ folder and for refactoring the help targets/comments used in the root makefile into cleaner and more readable commands.

The build targets also take in consideration the naming requirements for the binaries and allow them to be changed easily without having to change any package names.

Issue

Fixes #366

Type of change

Please mark the relevant option(s):

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Major breaking change
  • Documentation
  • Other

List of changes

  • Splits root Makefile into multiple files
  • Implements build targets to generate pocket binaries
  • Refactor pocket help Makefile targets and comments

Testing

  • make develop_test
  • LocalNet w/ all of the steps outlined in the README

Required Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have tested my changes using the available tooling
  • I have updated the corresponding CHANGELOG

If Applicable Checklist

  • I have updated the corresponding README(s); local and/or global
  • I have added tests that prove my fix is effective or that my feature works
  • I have added, or updated, mermaid.js diagrams in the corresponding README(s)
  • I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

@Gustavobelfort
Copy link
Contributor Author

Currently working in #366 (comment)

@Olshansk Olshansk self-requested a review December 12, 2022 21:48
@Olshansk
Copy link
Member

Olshansk commented Dec 13, 2022

@Gustavobelfort For future reference, please make sure to update all the "logistical" things:

  • Remove comments int he PR template
  • Update the issue & description
  • Labels
  • Project
  • Assignee
  • Etc...

E.g. the "list of changes" is actually the list of issues that should be in the Issue section. Please also be more descriptive with the list because you did A LOT and reading that before reviewing the code helps provide context.

Will let you give you a shot and lmk if you have any questions after updating

Screenshot 2022-12-12 at 7 26 49 PM

@Olshansk Olshansk mentioned this pull request Dec 13, 2022
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

@Gustavobelfort Looking really good so far! Took a first round of review with some comments and questions.

Makefile Show resolved Hide resolved
app/pocket/main.go Outdated Show resolved Hide resolved
build.mk Show resolved Hide resolved
Makefile Show resolved Hide resolved
build.mk Show resolved Hide resolved
build.mk Show resolved Hide resolved
build.mk Outdated Show resolved Hide resolved
build.mk Show resolved Hide resolved
build.mk Show resolved Hide resolved
build.mk Show resolved Hide resolved
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

@Gustavobelfort Looking really good so far! Took a first round of review with some comments and questions.

@Gustavobelfort Gustavobelfort self-assigned this Dec 13, 2022
@Gustavobelfort Gustavobelfort added the tooling tooling to support development, testing et al label Dec 13, 2022
@Gustavobelfort Gustavobelfort linked an issue Dec 13, 2022 that may be closed by this pull request
6 tasks
@Gustavobelfort Gustavobelfort changed the title FEAT: Create build makefile [Tooling] Create build makefile Dec 13, 2022
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

Thank you for updating the PR description!

Also, any reason to keep this as a draft?

app/pocket/main.go Outdated Show resolved Hide resolved
app/client/main.go Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
build.mk Show resolved Hide resolved
build.mk Outdated Show resolved Hide resolved
build.mk Show resolved Hide resolved
build.mk Show resolved Hide resolved
build.mk Show resolved Hide resolved
build.mk Show resolved Hide resolved
@Gustavobelfort Gustavobelfort marked this pull request as ready for review December 15, 2022 15:42
@Olshansk
Copy link
Member

@Gustavobelfort Let me know when this is ready for another look!

build.mk Show resolved Hide resolved
build.mk Outdated Show resolved Hide resolved
build.mk Outdated Show resolved Hide resolved
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

Looks great, thank you @Gustavobelfort! 🙌

What we usually do is "Squash & Merge" + Put the PR description as the body of the commit. Go for it :)

@Gustavobelfort Gustavobelfort merged commit 17a3724 into main Dec 22, 2022
@Gustavobelfort Gustavobelfort deleted the feat/package-binary branch December 22, 2022 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling tooling to support development, testing et al
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[CLI] package it and create a binary named p1
2 participants