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

onfluent-platform: fix confluent-platform cli #50592

Closed
wants to merge 2 commits into from

Conversation

bozzzzo
Copy link

@bozzzzo bozzzzo commented Feb 23, 2020

  • 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>)?

Confluent-platform install a confluent CLI that is not really working.
Two fixes are needed:

  • install the libexec folder
  • fix the confluent script to find the keg when run from /usr/local/bin

The `confluent` command is just a shell wrapper that
depends on the actual binary to reside in the libexec
directory of the distribution
The `confluent` shell script naively assumes it's run from the
distribution directory aka the keg and it fail when run as
`/usr/local/bin/confluent`.

Patch the script as it's not abvious where to submit the patch upstream.
@chenrui333 chenrui333 changed the title Fix confluent-platform confluent CLI onfluent-platform: fix confluent-platform cli Feb 23, 2020
Copy link
Member

@SMillerDev SMillerDev left a comment

Choose a reason for hiding this comment

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

.

rm_rf "#{libexec}/cli/linux_386"
rm_rf "#{libexec}/cli/linux_amd64"
rm_rf "#{libexec}/cli/windows_386"
rm_rf "#{libexec}/cli/windows_amd64"
end

test do
assert_match version.to_s, shell_output("#{bin}/kafka-broker-api-versions --version")
Copy link
Member

Choose a reason for hiding this comment

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

Could you improve this test to use some of the functionality of the software? Version tests rarely indicate that something is actually wrong. And especially if you're going to patch something we need a test.

@@ -11,14 +11,36 @@ class ConfluentPlatform < Formula

conflicts_with "kafka", :because => "kafka also ships with identically named Kafka related executables"

patch :DATA
Copy link
Contributor

Choose a reason for hiding this comment

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

Has this been reported upstream? This needs a comment and a link to an upstream issue at a minimum and description of when it should be removed or how it should be updated. In an ideal world, upstream would accept the fix and we would use a patch do block to grab the patch from github.

@zbeekman zbeekman added needs response Needs a response from the issue/PR author upstream issue An upstream issue report is needed labels Feb 23, 2020
@satyamagarwal
Copy link

I recently installed confluent-platform and I got this error :
confluentinc/cli crit executable /usr/local/bin/../libexec/cli/darwin_amd64/confluent does not exist. Make sure this script is up-to-date and resides at bin/confluent in the Confluent Platform release directory.

I think this pull request talks exactly about this ?

@bozzzzo
Copy link
Author

bozzzzo commented Feb 28, 2020

I think this pull request talks exactly about this ?

Yepyep.

Right now I don't have the time to do all the polishing required for this to be approved.
There is one further tweak missing, which is to do:

    export CONFLUENT_HOME=/usr/local/opt/confluent-platform

I believe there is a workaround, try running it as /usr/local/opt/confluent-platform/bin/confluent. If this works you can just put that path in front of /usr/local/bin, but it's clumsy.

@stale
Copy link

stale bot commented Mar 20, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale No recent activity label Mar 20, 2020
@stale stale bot closed this Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs response Needs a response from the issue/PR author stale No recent activity upstream issue An upstream issue report is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants