refactor LogStash::PluginManager::Update#validate_major_version

[colinsurprenant]

this method https://github.com/elastic/logstash/blob/028d76497ccef3a8d11a3528cdf6b99e8d10f070/lib/pluginmanager/update.rb#L70-L80 introduced by PR #3423 has a few issues:

• this method, like all other "lower level" methods which directly interact with Rubygems should be in the util.rb module.
• we should avoid, as much as possible, lazy require calls.
• I don't think we have prior-art for introducing an interactive Y/N in the logstash UI and I don't think this has been properly discussed in terms of strategy. what about automated updates? did we consider instead to introduce something like a --force options?
• the validate_major_version method name is confusing, you actually have to read the code to understand it.

[colinsurprenant]

this also relates to PR #3583 which introduces the --no-verify option. maybe just rely on that instead of the interactive Y/N ?

[suyograo]

I don't think we have prior-art for introducing an interactive Y/N in the logstash UI and I don't think this has been properly discussed in terms of strategy. what about automated updates? did we consider instead to introduce something like a --force options?

+1 on adding a --force or --no_prompt option to skip the [Y/N]. We also need to add this behavior in our docs

[colinsurprenant]

we should first make sure we do want to introduce interactive prompts in logstash (actually in the plugin manager) and if so provide a way to skip it. personally I'd prefer not to have interactive prompts and instead simply abort the process with the error and provide a way to force-update/skip the version validation.

[colinsurprenant]

The Y/N prompt is not integration-tests friendly as just discussed with @ph

[ph]

I like @talevy's idea to use -y flag, its similar to what debian aptitude uses.

[talevy]

++ to -y

[colinsurprenant]

@talevy @ph @suyograo any particular reason to keep an interactive prompt altogether? we don't have any other case of interactive prompts in logstash.

[jordansissel]

How about a --major-upgrade flag? Without it we log this error an exit with failure. Maybe?

[talevy]

@colinsurprenant a human should be aware that compatibility is not guaranteed, and this is really the only way to prevent a user from unintended, irreversible outcomes

[colinsurprenant]

@jordansissel +1

@talevy I am not following. it should simply fail by default with an appropriate error or proceed if given an override flag like --major-upgrade as suggested by @jordansissel

[suyograo]

@colinsurprenant There can be incompatible changes when a plugin gets updated (because of its own release cycle). As @talevy mentioned, a human should be able to intervene this update. We didn't have a precedent for this in LS before

[suyograo]

the default behavior can be Y/N prompts, but the integration tests and automated updates can use a --force_yes or some kind of a flag.

[colinsurprenant]

@suyograo for me this is the same as trying to install a plugin which does not have the proper metadata, it will fail, unless you pass the --no-verify flag. what's the difference here?

[andrewvc]

:+1: to @colinsurprenant let's keep the config options consistent

[jordansissel]

About --major upgrade - main idea was to target the flag at the specific concern. I dislike --force and --yes or similar because they don't have context as to what you are forcing or saying "yes" to.

[colinsurprenant]

For the interactive prompt: unless someone strongly disagree I suggest we move on and a) remove the interactive prompt b) by default fail on major version upgrade and c) add an override switch --major-upgrade.

[purbon]

I think there has been some clearance discussed here, I will try to provide a solution in a PR together with #3818, but still not sure about the fail on major upgrades? I like the idea of having a flag, and kinda dislike --force or --yes because of the context, .... what about no failing in case of major upgrade, but providing an option in case you want to be sure and apply this verification?

something like ---verify-major do you like this? I think this will provide an expected failure, I'm not sure I know other systems with restrictive upgrade policies like the one we're talking here.

[suyograo]

Related to https://github.com/elastic/logstash/issues/3993