-
Notifications
You must be signed in to change notification settings - Fork 855
Support for plugin diffs + improved testing on minor upgrades + fixes #269
Conversation
Ok the config test issues are due to a spelling mistake. The multi tests depend on jmespath - ill add to the provisonning and repush. |
@jakommo @strootman @jpcarey mind reviewing. |
- set_fact: list_command="list" | ||
- set_fact: list_command="" | ||
#If we are reinstalling all plugins, e.g. to a version change, we need to remove all plugins (inc. x-pack) to install any plugins. Otherwise we don't consider x-pack so the role stays idempotent. | ||
- set_fact: list_command="| grep -vE 'x-pack'" |
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.
@gingerwizard I dont get why you exclude x-pack here. Specially how this makes it idempotent.
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.
Normally we don't want to consider x-pack in deltas of plugins - as its handled by a seperate task later. However, if we are doing a complete re-install e.g. version change, we are forced to uninstall it in order to install any other plugins.
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.
The later x-pack task will inturn install it again. This was a workaround until we refactor the x-pack handling in the role - which id like to do but suspect it will be very disruptive.
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.
humm! understood. But for the x-pack installation, why can't we just add x-pack
to the es_plugins
list? In that way, all plugins are managed the same way and avoid code duplication.
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.
maybe latter with the x-pack refactoring? Or even let the user add into the plugins list he wants to manage.
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.
indeed thats the plan but it meant touching more beyond the remit of this PR. I think its reasonable to want to do this in one place though - although the x-pack stuff does alot more.
@jpcarey changes caused a build and 2 failures. These are hard to reproduce and seem to occur intermittently and are associated with templates and auth - this is subject to a rewrite shortly. Lets see if they occur, jenkins, test it |
Actually its because when we go up versions we have to uninstall x-pack which might be causing users to be dropped.... |
I would suggest we rename tests at some point to reflect their purpose. Standard should be enhanced to test more idempotent features. Also we can probably condense.