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 snabb config test functionality #1339

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

benagricola
Copy link
Contributor

@benagricola benagricola commented May 3, 2018

@wingo as mentioned on Slack earlier 🙂

Reworks snabb config command line parsing slightly to implement snabb config test.

snabb config test loads a schema passed with -s either by name (if already known by snabb) or by file path, then validates a passed configuration FILE against the schema.

If no -s is given but ID is passed, then config test attempts to connect to the instance and grab the current schema.

Also modifies the log() function so that informational log messages like the ones below are only printed when verbose is passed to yang.load_configuration:

config/snabb.conf: loading compiled configuration from config/snabb.o
config/snabb.conf: compiled configuration is up to date.

NOTE: WIP as I haven't tested this against running snabb instances yet, will complete this next week.

@benagricola benagricola force-pushed the config-test branch 2 times, most recently from 3a83c4f to 3b0e5f3 Compare May 9, 2018 09:35
Reworks `snabb config` command line parsing somewhat
to allow `snabb config test` to work.

`snabb config test` loads a schema passed with `-s` either by
name (if already known by snabb) or by file path, then validates
a passed configuration FILE against the schema.

Signed-off-by: Ben Agricola <[email protected]>
Copy link
Contributor

@wingo wingo left a comment

Choose a reason for hiding this comment

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

Looking good :) Some comments. Also, do you think it ever makes sense to use a compiled file? I have the feeling that we should always be "testing" source. (Should this command be named "snabb config compile" instead, and make it compile a configuration instead?)

@@ -53,8 +53,10 @@ function load_configuration(filename, opts)
end
local function err(msg, ...) error(err_msg(msg, ...)) end
local function log(msg, ...)
io.stderr:write(err_msg(msg, ...)..'\n')
io.stderr:flush()
if opts.verbose then
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to change the default above at line 34 to verbose={default=true}, to keep the current behavior for existing callers.

if opts.require_schema then err("missing --schema arg") end
ret.schema_name = descr.default_schema

if opts.command == 'test' then
Copy link
Contributor

Choose a reason for hiding this comment

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

I dunno, I think since there's these two ways to deal with the schema -- either as provided by the user or as received from the remote -- I think it makes sense to move all argument handling to test.lua, to avoid making this common.lua too specific on the commands being used. WDYT?

@eugeneia
Copy link
Member

eugeneia commented Jun 1, 2018

FWIW there is an ad-hoc implementation of this functionality in vita so this will be very useful to me! ❤️

@benagricola
Copy link
Contributor Author

@wingo re: should we ever config test a compiled file - no, that's a good point 😃- I'll rework this as config compile.

@lukego
Copy link
Member

lukego commented Jul 30, 2018

(JFYI I didn't merge this on my sweep through open PRs just now because based on the discussion it seems like changes are pending.)

@benagricola
Copy link
Contributor Author

Yep, pending a rewrite I'm hoping to get around to after more $work related things :D

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