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

add minimal configtest command #904

Merged
merged 1 commit into from
May 11, 2015

Conversation

josephholsten
Copy link
Contributor

Minimalist implementation of #901. Needs:

  • some help text
  • better error handling
  • test cases

Open questions:

  • should this be named consul configtest or some other color of bike shed? This is similar to apachectl configtest.
  • are there any other use cases that should be included in this? We can always add more later.

This was referenced May 4, 2015
helpText := `
Usage: consul configtest [options]

Test that configs are valid.
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a more verbose description? It would be helpful to explain what exactly the command does and why it is useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How's this sound?

Tests that configs are valid by attempting to parse them. Useful to ensure a configuration change will not cause consul to fail after a restart.

@ryanuber
Copy link
Member

ryanuber commented May 5, 2015

@josephholsten this looks much better, thanks! RE: command naming, I think configtest is fine. I left some minor comments, and it looks like we are missing the middleman documentation, so if you want to add that feel free. Otherwise, we can also take care of it after a merge.

@josephholsten
Copy link
Contributor Author

I'll squash commits once we're ready.

@josephholsten
Copy link
Contributor Author

@ryanuber care to review the code again? I'll get the middleman doc ready

@josephholsten josephholsten force-pushed the configtest-clean branch 4 times, most recently from 8cc0775 to 3e6f40e Compare May 5, 2015 19:59
@josephholsten
Copy link
Contributor Author

Should be fully tested, doc'd, squashed and ready to roll.


# Consul ConfigTest

The `consul configtest` command is the heart of Consul: it runs the agent that
Copy link
Member

Choose a reason for hiding this comment

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

This description needs some updating

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ha! updated.

@ryanuber
Copy link
Member

This LGTM. Thanks!

ryanuber added a commit that referenced this pull request May 11, 2015
@ryanuber ryanuber merged commit 3ad94bd into hashicorp:master May 11, 2015
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.

2 participants