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

mmctl 0.2.1 (new formula) #48814

Closed
wants to merge 1 commit into from
Closed

Conversation

ethervoid
Copy link
Contributor

  • 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?
  • Is your test running fine brew test <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>)?

@bayandin bayandin added the new formula PR adds a new formula to Homebrew/homebrew-core label Jan 9, 2020
Formula/mmctl.rb Show resolved Hide resolved
Formula/mmctl.rb Outdated Show resolved Hide resolved
Formula/mmctl.rb Outdated Show resolved Hide resolved
Formula/mmctl.rb Outdated Show resolved Hide resolved
@ethervoid
Copy link
Contributor Author

@SMillerDev Requested changes done :)

Formula/mmctl.rb Outdated Show resolved Hide resolved
@fxcoudert
Copy link
Member

Sounds brand new, and not yet meeting our notability criteria.

@elyscape
Copy link
Contributor

@ethervoid In case it would help any, I wrote a simpler formula that you can mine for ideas.

@ethervoid
Copy link
Contributor Author

@fxcoudert this CLI tool has been in Mattermost for a long time.

We just move it to a new project outside the Mattermost server in order to have an independent client and its widely used by people working with Mattermost. It would be great to have an easy way to install it through a powerful tool like brew :)

@ethervoid
Copy link
Contributor Author

@bayandin I've applied the required changes :)

ENV["GOBIN"] = buildpath/bin
ENV["ADVANCED_VET"] = "FALSE"
ENV["BUILD_HASH"] = Utils.popen_read("git rev-parse HEAD").chomp
ENV["BUILD_VERSION"] = version.to_s
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, it probably makes more sense to set make parameters in the call to make rather than as environment variables, with something like this:

ENV["GOBIN"] = buildpath/"bin"

hash = Utils.popen_read("git rev-parse HEAD").chomp
(buildpath/"src/github.com/mattermost/mmctl").install buildpath.children
cd "src/github.com/mattermost/mmctl" do
  system "make", "install", "ADVANCED_VET=FALSE", "BUILD_HASH=#{hash}", "BUILD_VERSION=#{version}"
end

Copy link
Contributor

Choose a reason for hiding this comment

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

Furthermore, you don't need to set BUILD_HASH here because the makefile will generate the same result if you don't specify a value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather prefer to pass them as environment variables instead of making a huge command because it's only calling one command so it's not going to create a problem there.

And I prefer to get the BUILD_HASH explicitly even though the Makefile does it, because the person that reads the code in the future knows where is being set and where that comes from.

@elyscape
Copy link
Contributor

Installing shell completions would be nice. That can be done with the following snippet at the end of the install stanza:

# Install bash completion
output = Utils.popen_read("#{bin}/mmctl completion bash")
(bash_completion/"mmctl").write output

# Install zsh completion
output = Utils.popen_read("#{bin}/mmctl completion zsh")
(zsh_completion/"_mmctl").write output

@ethervoid
Copy link
Contributor Author

@elyscape I don't like to install scripts in the users env without theire permission

@SMillerDev
Copy link
Member

It doesn't install to the user environment, it installs to a separate bash completion directory.

@ethervoid
Copy link
Contributor Author

@SMillerDev Yes, I know. What I mean is that I don't like to install more than the essential but if you think is good for the user and a best practice to have the Formulae in brew, I'll include the completion scripts installation

@SMillerDev
Copy link
Member

I don't think there's an official best practice but I've never seen anyone complain about being able to source completions.

@ethervoid
Copy link
Contributor Author

@SMillerDev In that case I'd rather prefer to add a new formulae called mmctl-completion like for example rails-completion, cargo-completion, etc

What else do I need to change to have this Formulae merged? Thank you!

@SMillerDev
Copy link
Member

Those are all seperate projects, homebrew doesn't split projects unless there's no other way. Please just add the bash completion.

@ethervoid
Copy link
Contributor Author

ethervoid commented Jan 13, 2020

@SMillerDev changes done and checks passed

Formula/mmctl.rb Outdated Show resolved Hide resolved
@SMillerDev
Copy link
Member

Thanks @ethervoid

@chenrui333 chenrui333 changed the title mmctl 0.2.1 mmctl 0.2.1 (new formula) Jan 17, 2020
@chenrui333
Copy link
Member

Thanks @ethervoid for your contribution to Homebrew! 🎉 🥇

Without awesome contributors like you, it would be impossible to maintain Homebrew to the high level of quality users have come to expect. Thank you!!!!

@ethervoid
Copy link
Contributor Author

@chenrui333 thank you very much! and thanks to all of you for working and maintaining this amazing tool :)

@chenrui333
Copy link
Member

Appreciate the kind words!

@lock lock bot added the outdated PR was locked due to age label Feb 16, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new formula PR adds a new formula to Homebrew/homebrew-core outdated PR was locked due to age ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants