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) SSL auto-generation + dnsmasq for new CLI #1299

Merged
merged 31 commits into from
Jun 27, 2016

Conversation

subnetmarco
Copy link
Member

@subnetmarco subnetmarco commented Jun 8, 2016

  • Brings back auto-generation of SSL certificates.
  • Brings back dnsmasq.
  • Brings back kong cluster .. CLI
  • Implements Closing #1255 #1281 (already implemented in next).
  • Bugfixes and other minor changes (like improving Serf info messages at startup).
  • Organizing pids, logs and Serf files respectively in pids, logs and serf folders.

@subnetmarco subnetmarco force-pushed the refactor/ssl-autogen branch 3 times, most recently from b35cdc9 to 9830033 Compare June 8, 2016 21:59
@subnetmarco subnetmarco force-pushed the refactor/ssl-autogen branch from 9830033 to 68a0c29 Compare June 8, 2016 22:17
@subnetmarco subnetmarco mentioned this pull request Jun 9, 2016
@subnetmarco subnetmarco changed the title Auto-Generating SSL certificates Refactor and fixes Jun 9, 2016

-- Check that the files exist
if not pl_path.exists(ssl_cert) then
return false, "Can't find SSL certificate at: "..ssl_cert
Copy link
Member

Choose a reason for hiding this comment

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

prefer returning nil, by convention with the rest of the codebase.

Copy link
Member

@thibaultcha thibaultcha Jun 10, 2016

Choose a reason for hiding this comment

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

also: to stay consistent, don't capitalize error messages. In the CLI, the errors always appear as:

Error: can't find SSL certificate at: ...

for _, cmd in ipairs(commands) do
local ok, _, _, stderr = pl_utils.executeex(cmd)
if not ok then
return nil, "There was an error when auto-generating the default SSL certificate: "..stderr
Copy link
Member

Choose a reason for hiding this comment

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

ditto (capitalized) + error could be shorter and consistent with the other errors of the CLI: "could not generate the default SSL cert:"

@@ -1,2 +1,3 @@
ssl = on
dnsmasq = on
dns_resolver = "8.8.8.8"
Copy link
Member

Choose a reason for hiding this comment

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

No need for quotes.

finally(function()
helpers.execute(KILL_ALL)
helpers.dir.rmtree("foobar")
end)
Copy link
Member

@thibaultcha thibaultcha Jun 16, 2016

Choose a reason for hiding this comment

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

Please be more careful, this test clearly belongs to a category of "errors" test, as stated by the scoping describe. If you change the behavior of the CLI (I won't even discuss the legitimacy of automatically creating a non-existing folder on behalf of the user, that is unexpected behavior to me), this is now testing the opposite. This test should be moved outside of the errors.

@jmdacruz
Copy link

Agree that passing configuration for stopping a service is not ideal (and it can also be error prone: what happens if somebody updates the configuration files at runtime and then trie to stop/start Kong?). You could store the configuration using nginx's PID as a key (e.g., create a folder for the PID somewhere and store the configuration files for the duration of the application's lifecycle)

@thibaultcha thibaultcha changed the title Refactor and fixes feat(cli) SSL auto-generation + dnsmasq for new CLI Jun 27, 2016
@thibaultcha thibaultcha merged commit 4eb34b0 into refactor/cli Jun 27, 2016
@subnetmarco subnetmarco deleted the refactor/ssl-autogen branch June 28, 2016 06:35
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.

4 participants