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

[feature] CLI add command (add-api, add-consumer) #147

Closed
wants to merge 3 commits into from

Conversation

thibaultcha
Copy link
Member

Part of improvements needed for the CLI described in #92. Sugar method of adding APIs and Consumers to Kong:

$ kong add-consumer --username jason
[WARN] No configuration at: /etc/kong/kong.yml using default config instead.
[INFO] Using configuration: /usr/local/lib/luarocks/rocks/kong/0.2.0-1/conf/kong.yml
[OK] Consumer added to Kong: username=jason created_at=1429732358000 id=bc7159a9-8c1b-4274-c284-364f64fe4481
$ kong add-api --name mockbin --public-dns mockbin.com --target-url http://mockbin.com
[WARN] No configuration at: /etc/kong/kong.yml using default config instead.
[INFO] Using configuration: /usr/local/lib/luarocks/rocks/kong/0.2.0-1/conf/kong.yml
[OK] API added to Kong: name=mockbin target_url=http://mockbin.com id=fb37eb8a-623c-4916-c923-976f5e523c69 public_dns=mockbin.com created_at=1429732399000

What could be improved:

  • Parsing of the arguments. Because of the library we are using, the way they are written in the examples above is the only way to send arguments. No --username='jason' for example.
  • Output maybe

@thibaultcha thibaultcha force-pushed the feature/cli-add branch 2 times, most recently from 3606195 to 2c08728 Compare April 21, 2015 23:29
@thibaultcha thibaultcha changed the title [feature] add-api command [feature] CLI add command (add-api, add-consumer) Apr 22, 2015
@thibaultcha
Copy link
Member Author

@thefosk thoughts? @nijikokun do you see this being simpler for the docs?

@nijikokun
Copy link
Contributor

so this starts an instance and executes, does it also check if an instance is already running?

@thibaultcha
Copy link
Member Author

It does not start any instance, just expects cassandra to be running.

@nijikokun
Copy link
Contributor

Then why the configuration output? I guess it is to tell you which instance its being done to?

@thibaultcha
Copy link
Member Author

Because it's mandatory like any Kong command, it needs the configuration, in this case for the cassandra properties.

@nijikokun
Copy link
Contributor

Hmm, seems a bit verbose, but alright.

Output could be a little bit better, on multiple lines for the data so its easier to read, and spot potential mistakes.

I think it would also require update and remove.


Something I was thinking of was kong api --add [type] [arguments] perhaps its too much, was also thinking about kong (add,update,remove) but that could conflict with future commands.

@thibaultcha
Copy link
Member Author

We don't need to deal with the API so let's not bother. kong add [type] [arguments] could be enough. But because of the way the CLI is built, I wanted to have smaller commands, with a smaller purpose. Hence why add-api and add-consumer.

I think update is overkill, remove why not.

@thibaultcha
Copy link
Member Author

It's only verbose because I'm using the utils method to retrieve a configuration, and it outputs logs when it runs, only to inform the user like the other commands. If it's verbose here, it might be in other commands too.

@thibaultcha thibaultcha force-pushed the feature/cli-add branch 4 times, most recently from 7dbb819 to ee352b0 Compare April 28, 2015 14:02
@thibaultcha thibaultcha mentioned this pull request Apr 29, 2015
5 tasks
@thibaultcha thibaultcha force-pushed the feature/cli-add branch 2 times, most recently from 8a197fe to 8f3e970 Compare April 30, 2015 01:19
@@ -5,5 +5,5 @@ source ./versions.sh
CASSANDRA_BASE=apache-cassandra-$CASSANDRA_VERSION

sudo rm -rf /var/lib/cassandra/*
curl http://www.us.apache.org/dist/cassandra/$CASSANDRA_VERSION/$CASSANDRA_BASE-bin.tar.gz | tar xz
curl http://apache.spinellicreations.com/cassandra/$CASSANDRA_VERSION/$CASSANDRA_BASE-bin.tar.gz | tar xz
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure it's a good idea to use none official sources?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is official, it is a mirror on the official download page. This change shouldn't be here anyways, oops.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, I just saw the domain and it looks like some guy's personal site ...

Copy link
Member Author

Choose a reason for hiding this comment

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

What actually happened is that some day, all of a sudden, version 2.1.{3 I think} just disappeared from the main repo, without any notice... I used this mirror which still had it but after a while it disappeared too. Turns out we don't have a choice but to update to 2.1.{4,5}, which we did already in master.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah-mazing!

@thibaultcha thibaultcha force-pushed the feature/cli-add branch 3 times, most recently from 3306b8a to 4ae27e5 Compare May 14, 2015 13:44
@thibaultcha thibaultcha added the pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) label Jun 5, 2015
@thibaultcha
Copy link
Member Author

Added a ready label because as discussed with @sinzone, this is ready to be merged anytime as is. more can be added to it later.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 62.12% when pulling d2ebdc9 on feature/cli-add into 486f374 on master.

@sonicaghi
Copy link
Member

@thibaultcha remember to merge this before 0.4

@thibaultcha
Copy link
Member Author

That's not as easily doable as before because the CLI arguments parsing expects one letter args names (-n for --name) but now that an API can have a public_dns and a path, we can't have two -p arguments. And it doesn't accept --path and --public_dns. I would like to find another parsing library/script and implement this for more entities, as well as maybe adding more interactions

@thibaultcha thibaultcha closed this Jul 8, 2015
@thibaultcha thibaultcha deleted the feature/cli-add branch July 8, 2015 13:45
bungle added a commit that referenced this pull request May 23, 2022
### Summary

> Released on: 2022/05/20

#### Fixed

- Improve DC-aware LB policies robustness.
  [#147](thibaultcha/lua-cassandra#147)
- Ensure request-aware + DC-aware LB policy prioritizes local peers over remote
  ones.
bungle added a commit that referenced this pull request May 24, 2022
### Summary

> Released on: 2022/05/20

#### Fixed

- Improve DC-aware LB policies robustness.
  [#147](thibaultcha/lua-cassandra#147)
- Ensure request-aware + DC-aware LB policy prioritizes local peers over remote
  ones.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants