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

k9s 0.21.2 (new formula) #57432

Closed
wants to merge 1 commit into from

Conversation

crunchtime-ali
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>)?

k9s is an insanely handy and popular interactive CLI to deal with Kubernetes clusters. There is an official formula available here https://github.com/derailed/homebrew-k9s/blob/master/Formula/k9s.rb but I think it should be added to the core repo for more exposure.

@dawidd6 dawidd6 added the new formula PR adds a new formula to Homebrew/homebrew-core label Jul 4, 2020
@crunchtime-ali
Copy link
Contributor Author

@dawidd6 Oh you introduced mandatory licence specification. This is troublesome in this case because when I specify `license Apache-2.0" I get the error

License mismatch - GitHub license is: NOASSERTION, but Formulae license states: Apache-2.0

What should I do now? In this case https://github.com/derailed/k9s/blob/master/LICENSE is the Apache-2.0 license.

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.

Please add the license statement regardless and we'll check if that goes wrong

Formula/k9s.rb Outdated
end

test do
system "k9s", "version"
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
Contributor Author

Choose a reason for hiding this comment

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

Hey @SMillerDev providing a meaningful assertion might be really difficult as k9s only has 3 commands. The closest one to making any sense is just launching the interactive k9s without a command and checking for the the text Boom!! Invalid kubeconfig context detected. However that test will fail on a regular users machine where a Kubernetes config file exists. In that case the interactive CLI would be launched.

image

Copy link
Member

Choose a reason for hiding this comment

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

If it needs to be in $HOME/.k9s/config.yml you can set ENV['HOME'] to testpath since that will be an empty directory. That way it'll always fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SMillerDev I added a test for the last line as well as a thorough explanation on the expected Kubernetes configuration. k9s needs a Kubernetes config, not its own one to run. Most tools around Kubernetes use either ~/.kube/config or the KUBECONFIG environment variable.
If at some point there is a chance to create a better test I will do so.

@crunchtime-ali
Copy link
Contributor Author

Please add the license statement regardless and we'll check if that goes wrong

I added the License back again.

@dawidd6 Wow, you are acting at lightspeed. It seems to be the right decision to not fail when GitHub does not return anything meaningful. I just created a new repository without a LICENSE file. Then I created a LICENSE file with exactly the same content of the k9s repo LICENSE file. Suddenly GitHub detects it as Apache License 2.0

@crunchtime-ali crunchtime-ali force-pushed the add-k9s branch 2 times, most recently from 1d7ed4e to 7bf066e Compare July 4, 2020 17:50
Copy link
Member

@chenrui333 chenrui333 left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@chenrui333 chenrui333 added the go Go use is a significant feature of the PR or issue label Jul 5, 2020
@BrewTestBot
Copy link
Member

:shipit: @chenrui333 has triggered a merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Go use is a significant feature of the PR or issue new formula PR adds a new formula to Homebrew/homebrew-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants