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.

Sign up for GitHub

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

Jump to bottom

kfctl 1.1.0 (new formula) #65230

Closed
wants to merge 9 commits into from
Closed

kfctl 1.1.0 (new formula) #65230

Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5854acf
New formula for kfctl
naveensrinivasan Nov 19, 2020
10494c8
Fixed the tests
naveensrinivasan Nov 19, 2020
5d12daa
Fixed the formatting issue.
naveensrinivasan Nov 19, 2020
c70ccaf
More formatting changes.
naveensrinivasan Nov 19, 2020
0fdfab5
Some more formatting.
naveensrinivasan Nov 19, 2020
276577e
Moved to the folder.
naveensrinivasan Nov 19, 2020
51ce998
Moved to the folder.
naveensrinivasan Nov 19, 2020
f950f54
Merge branch 'master' of https://github.com/Homebrew/homebrew-core in…
naveensrinivasan Nov 20, 2020
5f91c98
Formatted the content using brew audit
naveensrinivasan Nov 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions Formula/kfctl.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
class Kfctl < Formula
desc "Kubeflow command-line interface"
homepage "https://github.com/kubeflow/kfctl"
url "https://github.com/kubeflow/kfctl.git",
tag: "v1.1.0",
revision: "9a3621e2b2ce6d2baa5c78acccec5028c4e4cbe1"
license "Apache-2.0"

depends_on "go@1.13" => :build
Copy link
Member

Choose a reason for hiding this comment

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

Does it not work with the most recent go?

Copy link
Author

Choose a reason for hiding this comment

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

It doesn't work. https://github.com/kubeflow/kfctl/blob/master/Makefile#L16

Copy link
Member

Choose a reason for hiding this comment

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

That doesn't mean it won't work with a newer version, but it does make me hesitant to accept it into homebrew, since apparently it's not trying to stay compatible with new releases, which in turn will cause problems for homebrew down the line. In februari this formula will break at this rate.

Copy link
Author

Choose a reason for hiding this comment

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

I have tried compiling it in the latest version and it has an issue and that is the reason I had to fall back to 1.13

Copy link
Author

Choose a reason for hiding this comment

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

So do you expect the upstream to fix this before coming with a brew formula?

Copy link
Member

Choose a reason for hiding this comment

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

So do you expect the upstream to fix this before coming with a brew formula?

I expect upstream to not be a year behind in security updates. If there is no work being done to keep up to date I'd consider it abandoned and violating the rule:

https://docs.brew.sh/Acceptable-Formulae#niche-or-self-submitted-stuff

be maintained (i.e. the last release wasn’t ages ago, it works without patching on all supported macOS releases and has no outstanding, unpatched security vulnerabilities)

Copy link
Author

Choose a reason for hiding this comment

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

I agree, closing it waiting for the upstream.


def install
system "make", "build"
bin.install "bin/darwin/kfctl"

# Install bash completion
output = Utils.safe_popen_read("#{bin}/kfctl", "completion", "bash")
(bash_completion/"kfctl").write output

# Install zsh completion
output = Utils.safe_popen_read("#{bin}/kfctl", "completion", "zsh")
(zsh_completion/"kfctl").write output
end

test do
run_output = shell_output("#{bin}/kfctl version 2>&1")
Copy link
Member

Choose a reason for hiding this comment

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

We need a test that exercises the some of the functionality of the app. Version checks or usage checks (foo --version or foo --help) are not sufficient, as explained in the formula cookbook.

In most cases, a good test would involve running a simple test case: run #{bin}/foo input.txt.

  • Then you can check that the output is as expected (with assert_equal or assert_match on the output of shell_output)
  • You can also check that an output file was created, if that is expected: assert_predicate testpath/"output.txt", :exist?

Some advice for specific cases:

  • If the formula is a library, compile and run some simple code that links against it. It could be taken from upstream's documentation / source examples.
  • If the formula is for a GUI program, try to find some function that runs as command-line only, like a format conversion, reading or displaying a config file, etc.
  • If the software cannot function without credentials, a test could be to try to connect with invalid credentials (or without credentials) and confirm that it fails as expected.
  • Same if the software requires a virtual machine, docker instance, etc. to be running.

Copy link
Author

Choose a reason for hiding this comment

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

This is similar to kubectl and that is the reason for to have contrived tests similar to kubectl https://github.com/Homebrew/homebrew-core/blob/cf898a530a161125d5d15146fc16f5074c6372dd/Formula/kubernetes-cli.rb

Copy link
Member

Choose a reason for hiding this comment

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

That does have an additional test to test the functionality, this is not good enough

Copy link
Author

Choose a reason for hiding this comment

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

OK, So do you have any suggestions? This is a CLI tool for API.

Copy link
Member

Choose a reason for hiding this comment

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

There's suggestions in my initial comment

Copy link
Author

Choose a reason for hiding this comment

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

I disagree with the recommendations.

If the software cannot function without credentials, a test could be to try to connect with invalid credentials (or without credentials) and confirm that it fails as expected.

  • What is the point of tests that are dependent on the API when it is not available?
  • How is this different from kubectl? I am trying to follow similar to kubectl.

Copy link
Member

Choose a reason for hiding this comment

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

What is the point of tests that are dependent on the API when it is not available?

They test a functional part of the application.

How is this different from kubectl? I am trying to follow similar to kubectl.

It's not the same tool

assert_match "kfctl v1.1.0-0-g9a3621e", run_output
end
end