-
Notifications
You must be signed in to change notification settings - Fork 31
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
@ #493 | should validate plugin params before proceeding #494
@ #493 | should validate plugin params before proceeding #494
Conversation
@hoatle I checked every case, and now the plugin configuration will work properly. |
lib/teracy-dev/plugin.rb
Outdated
reload_required = true | ||
name_validate = validate(plugin, 'name' => plugin['name']) | ||
if !name_validate | ||
logger.warn("Plugin name must be configured, not nil, and not empty") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Plugin name must be configured"
lib/teracy-dev/plugin.rb
Outdated
if installed_plugins.empty? | ||
source_validate = validate(plugin, 'sources' => plugin['sources']) | ||
if !source_validate | ||
logger.warn("Plugin sources should be configured when no plugin installed, or we will use default") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this warn
lib/teracy-dev/plugin.rb
Outdated
reload_required = true | ||
end | ||
else | ||
logger.debug('Plugin state not be set, do nothing') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when plugin.enabled is true
and state
is not setted, we should fill it with installed
state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plugin state is not set, then do nothing is expected. It means that if the plugin is installed or not installed, users don't care.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okey, by the way, the message should be "The plugin state is not set, no action will be taken."
2b98cd7
to
c556f84
Compare
@datphan I've updated! please check again. |
c556f84
to
7ff558d
Compare
7ff558d
to
8a28422
Compare
lib/teracy-dev/plugin.rb
Outdated
@@ -45,5 +58,33 @@ def self.sync(plugins) | |||
def self.installed?(plugin_name) | |||
return Vagrant.has_plugin?(plugin_name) | |||
end | |||
|
|||
def self.validate(plugin, config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't introduce this type of complexity, as I can see it just checks the existence. So use Util.exist? instead
lib/teracy-dev/plugin.rb
Outdated
plugin_manager.install_plugin(plugin['name'], Util.symbolize(plugin)) | ||
reload_required = true | ||
name_validate = validate(plugin, 'name' => plugin['name']) | ||
if !name_validate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unless Util.exist? plugin['name']
is enough for example
8a28422
to
fc1d021
Compare
@hoatle I've updated! please review again! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
connect #493
please review this PR!