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

feat(cli) kong prepare cli command to prepare prefix #2706

Merged
merged 3 commits into from
Jul 24, 2017

Conversation

subnetmarco
Copy link
Member

@subnetmarco subnetmarco commented Jul 18, 2017

Full changelog

  • Introduces a new kong prepare command that only prepares the prefix.

Documentation at Kong/docs.konghq.com@c5b332e

Copy link
Member

@thibaultcha thibaultcha left a comment

Choose a reason for hiding this comment

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

Cool stuff 👍 The implementation is fine but there are some things we can definitely improve here - including the tests that seem rather sparse.

prefix = args.prefix
}))

local ok, err = prefix_handler.prepare_prefix(conf, args.nginx_conf)
Copy link
Member

Choose a reason for hiding this comment

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

I think we are missing this nginx-conf argument from the command's options?

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in last commit.

local lapp = [[
Usage: kong prepare [OPTIONS]

Prepares the Kong prefix in the configured prefix directory.
Copy link
Member

Choose a reason for hiding this comment

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

The CLI's help messages do not use the third person. Should be Prepare the Kong prefix...

Copy link
Member

Choose a reason for hiding this comment

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

The help message should also include an Example usage like the kong compile command does, to help the user understand the use-case that this command solves:

Prepare the Kong prefix in the configured prefix directory. This command can
be used to start Kong from the nginx binary without using the 'kong start' command.

Example usage:
  kong prepare -p /usr/local/kong -c kong.conf && nginx -p /usr/local/kong -c nginx.conf

local ok, err = prefix_handler.prepare_prefix(conf, args.nginx_conf)
if not ok then
log.verbose("could not prepare Kong")
error(err) -- report to main error handler
Copy link
Member

Choose a reason for hiding this comment

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

A better experience would be to log:

if not ok then
  error("could not prepare Kong prefix at " .. conf.prefix .. 
        ": " .. err)
end

@thibaultcha thibaultcha added the pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. label Jul 18, 2017
@thibaultcha thibaultcha added this to the 0.11.0rc2 milestone Jul 18, 2017
Copy link
Member

@Tieske Tieske left a comment

Choose a reason for hiding this comment

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

lgtm, but... wasn't this supposed to replace the compile command? So shouldn't that one be removed?

@subnetmarco
Copy link
Member Author

subnetmarco commented Jul 18, 2017

@Tieske we decided to keep both for the time being, but deprecate compile and get rid of it later.

@subnetmarco subnetmarco added pr/status/please review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Jul 18, 2017
@Tieske
Copy link
Member

Tieske commented Jul 19, 2017

@thefosk ok, but then a deprecation warning should be added to the docs

@subnetmarco
Copy link
Member Author

@Tieske Deprecated with Kong/docs.konghq.com@a68553c

WARN logs used to be printed on stdout instead of stderr. This makes
WARN logs conflict with commands whose output from stdout (like 'kong compile')
is used to generate something.
@thibaultcha thibaultcha merged commit dbe04cd into next Jul 24, 2017
@thibaultcha thibaultcha deleted the feat/cli-prepare branch July 24, 2017 20:00
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.

3 participants