-
Notifications
You must be signed in to change notification settings - Fork 56
(PE-40375) - Phase 1 - Pre-migration checks #558
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
Conversation
…. pushing change to a comment to re-trigger them because goign into every test to hit re-run was tediuos
plans/migrate.pp
Outdated
| # pre-migration checks | ||
| out::message('This plan is a work in progress and it is not recommended to be used until it is fully implemented and supported') | ||
| peadm::assert_supported_bolt_version() | ||
| peadm::assert_supported_pe_version($pe_version, $permit_unsafe_versions) |
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.
This line will fail as the params $pe_version and $permit_unsafe_versions are not defined. For $pe_version we can use $upgrade_version but we need to define $permit_unsafe_versions and set it to either true or false (search through the codebase for other instances of 'peadm::assert_supported_pe_version' to see what is generally passed in for $permit_unsafe_versions). We should also wrap that line in an 'if' statement like we do near the bottom of this file:
if $upgrade_version and $upgrade_version != '' and !empty($upgrade_version) {
peadm::assert_supported_pe_version($pe_version, $permit_unsafe_versions)
}
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.
Still failing with: Evaluation Error: Unknown variable: 'pe_version'. (file: /home/runner/work/puppetlabs-peadm/puppetlabs-peadm/spec/fixtures/modules/peadm/plans/migrate.pp, line: 22, column: 38)
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.
found the problem: in assert_supported_pe_version being called the variable is just called $version not $pe_version
…t to use an if statement to check for upgrade version first * Set to false as that’s the default in assert_supported_pe_version.pp * Run same verify can connect to all hosts on if it exists
plans/migrate.pp
Outdated
| peadm::assert_supported_bolt_version() | ||
| if $upgrade_version and $upgrade_version != '' and !empty($upgrade_version) { | ||
| $permit_unsafe_versions = false | ||
| peadm::assert_supported_pe_version($version, $permit_unsafe_versions) |
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.
looks like $version still needs to be updated to $upgrade_version here?
…sion if applicable * modifying the check for a replica host when verifying host connections to be cleaner
Summary
Adding initial versions of pre-migration checks. Do the hostnames exist, can we connect to them, is a valid PE version used, is original cluster operational, is it a supported PE architecture
Additional Context
Add any additional context about the problem here.
Related Issues (if any)
Mention any related issues or pull requests.
Checklist
Changes include test coverage?
Have you updated the documentation?