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

Use shards command with production flag #670

Merged
merged 3 commits into from
Mar 7, 2018
Merged

Conversation

faustinoaq
Copy link
Contributor

@faustinoaq faustinoaq commented Mar 4, 2018

Description of the Change

Add --production flag and avoid using @crystal deps , use @shards install instead

Alternate Designs

No

Benefits

Avoid running ameba when running make to build amber executable

Possible Drawbacks

No

@faustinoaq faustinoaq requested a review from a team March 4, 2018 00:29
@faustinoaq
Copy link
Contributor Author

@amberframework/core-team Some suggestions? 😅

@marksiemers
Copy link
Contributor

This seems fine to me, but I don't know enough about the differences between the two commands to approve.

@robacarp
Copy link
Member

robacarp commented Mar 6, 2018

I'm in the same boat as @marksiemers.

@faustinoaq
Copy link
Contributor Author

This seems fine to me, but I don't know enough about the differences between the two commands to approve.

@marksiemers @robacarp crystal deps has been removed here crystal-lang/crystal#5544

So, we should use shards install instead

Oh, and --production flag is to avoid installing ameba, because isn't needed here 😅

Copy link
Member

@elorest elorest left a comment

Choose a reason for hiding this comment

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

Yeah I agree. Crystal deps probably shouldn't be used.

@faustinoaq faustinoaq merged commit 5dd38c7 into master Mar 7, 2018
@faustinoaq faustinoaq deleted the fa/shards-production branch March 7, 2018 12:45
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.

4 participants