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

easyengine 4.0.1 (new formula) #34378

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@mbtamuli
Contributor

mbtamuli commented Nov 21, 2018

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

Signed-off-by: Mriyam Tamuli mbtamuli@gmail.com

New formula: easyengine
Signed-off-by: Mriyam Tamuli <mbtamuli@gmail.com>
@mbtamuli

This comment has been minimized.

Contributor

mbtamuli commented Nov 22, 2018

@lembacon The build seems to have passed. Do you need anything else from my end to merge this?

@SMillerDev

This comment has been minimized.

Contributor

SMillerDev commented Nov 22, 2018

Does this require both PHP and Docker?

end
test do
system bin/"ee cli version"

This comment has been minimized.

@SMillerDev

SMillerDev Nov 22, 2018

Contributor

Could you extend this test to test more functionality of the program?

This comment has been minimized.

@PatelUtkarsh

PatelUtkarsh Nov 22, 2018

Since we have dependency on docker for all current feature set, other feature test case will not able to pass in CI.

We can add more test-cases in future when we have more command which don’t depend on docker. @SMillerDev

This comment has been minimized.

@javian

javian Nov 22, 2018

Member

I don't think this would be suitable for the core tap in that case especially since we can't get it to work without doing additional commands and not have it tested. You already have a viable way of installing it as shown on the website so I would stick to that for now imo.

@PatelUtkarsh

This comment has been minimized.

PatelUtkarsh commented Nov 22, 2018

@SMillerDev

Does this require both PHP and Docker?

Yep. The project is in PHP language so PHP is a dependency.
Also Docker is required as the entire project revolves around it.

@SMillerDev

This comment has been minimized.

Contributor

SMillerDev commented Nov 23, 2018

@Homebrew/core what is the opinion on formula requiring docker and not being usable otherwise?

@MikeMcQuaid

This comment has been minimized.

Member

MikeMcQuaid commented Nov 23, 2018

@SMillerDev I'm not sure I understand why a .phar or a macOS executable would require Docker. If you need Docker anyway: why aren't you installing it as part of the Dockerfile?

@mbtamuli

This comment has been minimized.

Contributor

mbtamuli commented Nov 23, 2018

@MikeMcQuaid The EasyEngine project itself is a written in PHP. It doesn't require docker to run. Put it other way, if docker is not installed, it will just print a message informing that and exit.

I guess our comments were not clear enough and caused confusion. Apologize for that.

EasyEngine itself is a simple phar file like wp-cli - https://github.com/Homebrew/homebrew-core/blob/master/Formula/wp-cli.rb

In fact, EasyEngine has its roots in WP-CLI project.

EasyEngine has some part which runs even without Docker. We will update PR to have those part of brew's test cases.

To summarize:
Docker is not hard dependency for EasyEngine itself. If Docker doesn't exist EasyEngine won't crash.

@MikeMcQuaid

This comment has been minimized.

Member

MikeMcQuaid commented Nov 23, 2018

Ok. I think the caveats can be removed then, thanks.

Add more tests and remove caveats
Signed-off-by: Mriyam Tamuli <mbtamuli@gmail.com>
@mbtamuli

This comment has been minimized.

Contributor

mbtamuli commented Nov 23, 2018

@MikeMcQuaid Thanks. I have updated the PR. Removed the caveats and added more tests.

CC: @SMillerDev

@mbtamuli

This comment has been minimized.

Contributor

mbtamuli commented Nov 23, 2018

@MikeMcQuaid @SMillerDev Not sure what went wrong. The build failed for the mojave configuration due to what seems like network issue.

@javian

This comment has been minimized.

Member

javian commented Nov 24, 2018

Yes there's a currently an issue with the CI which we haven't resolved yet.

@javian

This comment has been minimized.

Member

javian commented Nov 24, 2018

@BrewTestBot test this please

@mbtamuli

This comment has been minimized.

Contributor

mbtamuli commented Nov 24, 2018

@javian Thanks for running the tests again. They seems to have passed. 🙂

Show resolved Hide resolved Formula/easyengine.rb Outdated
Replace hardcoded version with #{version}
Signed-off-by: Mriyam Tamuli <mbtamuli@gmail.com>
@mbtamuli

This comment has been minimized.

Contributor

mbtamuli commented Nov 26, 2018

@javian Is there anything remaining for this to get merged?

@fxcoudert

This comment has been minimized.

Member

fxcoudert commented Nov 27, 2018

Looking good to me! @javian letting you squash and merged this :)

@javian javian changed the title from New formula: easyengine to easyengine 4.0.1 (new formula) Nov 28, 2018

@javian

This comment has been minimized.

Member

javian commented Nov 28, 2018

Done and dusted, thanks @mbtamuli !

@javian javian closed this in 7eda8ba Nov 28, 2018

@mbtamuli

This comment has been minimized.

Contributor

mbtamuli commented Nov 28, 2018

Thanks for all your suggestions to get this merged. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment