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

Allow optional namespaces value when calling kubectl apply. #1

Merged
merged 1 commit into from
Oct 31, 2019

Conversation

olymk2
Copy link
Contributor

@olymk2 olymk2 commented Oct 9, 2019

I had a stab at adding namespace support to kube ctl, Implemented in a way it should not effect existing functionality.

Still quite new to clojure so if I am doing anything glaringly bad let me know :)

@olymk2
Copy link
Contributor Author

olymk2 commented Oct 15, 2019

@brosenan are you accepting pr's ? just updated this to allow for kubeconfig param ass well as namespace.

@brosenan
Copy link
Owner

Sorry for the delay in my response.

The build for this PR is failing. I think the root cause is out-of-date dependencies. I did a quick fix, setting the jdk version to openjdk9 to make head build again.

Please rebase this PR and I'll accept it.

@brosenan
Copy link
Owner

Please add tests + documentation for the new parameters. LMK if you need any assistance.

;; `kube-apply` takes a string constructed by `to-yaml` and a `.yaml`

@olymk2 olymk2 force-pushed the namespace-handling branch 2 times, most recently from 965f731 to 7f8f775 Compare October 29, 2019 21:44
@olymk2
Copy link
Contributor Author

olymk2 commented Oct 29, 2019

@brosenan I had a little stab at adding a test and some text to the readme, need to look into the fact library at some point its layout is a bit foreign to me.

Copy link
Owner

@brosenan brosenan left a comment

Choose a reason for hiding this comment

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

Please run ./create-docs.sh to update the .md files. They should be part of the PR.

Thanks,
Boaz.

@@ -1307,6 +1307,7 @@ spec:
")

;; `kube-apply` takes a string constructed by `to-yaml` and a `.yaml`
;; optionally supply namespace and kube config parameters.
Copy link
Owner

Choose a reason for hiding this comment

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

This line seems to be out of place. It is in the middle of a sentence...

I would move it to just before line 1323, to describe the new test you created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay made those changes.

Copy link
Owner

@brosenan brosenan left a comment

Choose a reason for hiding this comment

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

Hi Oliver, thanks for your effort, and thanks for baring with me.

I just realized you made manual changes to core.md, and parallel changes to core_test.clj.

I'm using an approach in which the documentation (.md files) are directly derived from the tests (_test.clj files). Comments are taken to be free text and Clojure code is taken as code fragments.

This process is done automatically using the ./create-docs.sh script.

For this reason, the comments in core_test.clj are supposed to document the implementation, not the test. The line you wrote:

optionally supply namespace and kube config parameters.

would make a great comment for the test you have added (instead of the documentation you added in core_test.clj, which describes the test and not the feature you implemented.

Once you made the change, run ./create_docs.sh to generate the docs. If you have trouble running it on your machine (e.g., you're using Windows and have no sh/awk), let me know and I'll merge the code and run the script for you.

Thanks again,
Boaz.

@olymk2
Copy link
Contributor Author

olymk2 commented Oct 31, 2019

okay will change that when i get 2 secs, never seen docs generated from tests before, I had assumed incorrectly they where comment related to tests, does that ean you cant comment your tests ?

@olymk2
Copy link
Contributor Author

olymk2 commented Oct 31, 2019

@brosenan okay modified, I may have run the create docs script before modifying the tests, I had not realized the md files was generated from the tests comments, thanks for guiding me I may have a few more pr's going forward so this is helpful :)

@brosenan
Copy link
Owner

okay will change that when i get 2 secs, never seen docs generated from tests before, I had assumed incorrectly they where comment related to tests, does that ean you cant comment your tests ?

If the test is not self explanatory, you can add comments inside the (fact) form.

@brosenan brosenan merged commit 512753e into brosenan:master Oct 31, 2019
@olymk2 olymk2 deleted the namespace-handling branch October 31, 2019 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants