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

Module: Adding OpenBSD sysupgrade support #341

Merged
merged 1 commit into from
Aug 18, 2020

Conversation

precurse
Copy link
Contributor

@precurse precurse commented May 14, 2020

SUMMARY

This module will let users use OpenBSD's sysupgrade functionality with Ansible. This command is similar to syspatch, which is already in this repo, but handles upgrades between releases and snapshots.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

sysupgrade

ADDITIONAL INFORMATION

This module doesn't currently allow for check_mode, as the script within OpenBSD doesn't currently allow support for checking if a version is available. This will likely be resolved in a future release.

- name: Upgrade to latest release
  sysupgrade:
  become: true
  register: sysupgrade

- name: Upgrade to latest snapshot
  sysupgrade:
    snapshot: true
  register: sysupgrade

- name: Have Ansible reboot if required
  reboot:
  when: sysupgrade.changed

# Note: Ansible will error when running this way due to how
#   the reboot is forcefully handled by sysupgrade:

- name: Have sysupgrade automatically reboot
  sysupgrade:
    fetch_only: false
  become: true
  register: sysupgrade
  ignore_errors: true

@ansibullbot
Copy link
Collaborator

The test ansible-test sanity --test validate-modules [explain] failed with 4 errors:

plugins/modules/system/sysupgrade.py:0:0: parameter-type-not-in-doc: Argument 'fetch_only' in argument_spec defines type as 'bool' but documentation doesn't define type
plugins/modules/system/sysupgrade.py:0:0: parameter-type-not-in-doc: Argument 'force' in argument_spec defines type as 'bool' but documentation doesn't define type
plugins/modules/system/sysupgrade.py:0:0: parameter-type-not-in-doc: Argument 'keep_files' in argument_spec defines type as 'bool' but documentation doesn't define type
plugins/modules/system/sysupgrade.py:0:0: parameter-type-not-in-doc: Argument 'snapshot' in argument_spec defines type as 'bool' but documentation doesn't define type

click here for bot help

@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR community_review and removed community_review ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels May 14, 2020
plugins/modules/system/sysupgrade.py Outdated Show resolved Hide resolved
plugins/modules/system/sysupgrade.py Outdated Show resolved Hide resolved
plugins/modules/system/sysupgrade.py Outdated Show resolved Hide resolved
plugins/modules/system/sysupgrade.py Outdated Show resolved Hide resolved
plugins/modules/system/sysupgrade.py Outdated Show resolved Hide resolved
plugins/modules/system/sysupgrade.py Outdated Show resolved Hide resolved
plugins/modules/system/sysupgrade.py Outdated Show resolved Hide resolved
@ansibullbot ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed community_review labels May 15, 2020
@precurse precurse requested a review from Akasurde May 15, 2020 18:12
@Akasurde
Copy link
Member

LGTM, @felixfontein @Andersson007 Could you please review this? Thanks in advance.

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

This needs some sort of tests (I guess unit tests would be best, since CI doesn't have OpenBSD support).

plugins/modules/system/sysupgrade.py Outdated Show resolved Hide resolved
plugins/modules/system/sysupgrade.py Outdated Show resolved Hide resolved
plugins/modules/system/sysupgrade.py Outdated Show resolved Hide resolved
plugins/modules/system/sysupgrade.py Outdated Show resolved Hide resolved
plugins/modules/system/sysupgrade.py Outdated Show resolved Hide resolved
plugins/modules/system/sysupgrade.py Outdated Show resolved Hide resolved
plugins/modules/system/sysupgrade.py Outdated Show resolved Hide resolved
plugins/modules/system/sysupgrade.py Outdated Show resolved Hide resolved
plugins/modules/system/sysupgrade.py Outdated Show resolved Hide resolved
plugins/modules/system/sysupgrade.py Outdated Show resolved Hide resolved
plugins/modules/system/sysupgrade.py Outdated Show resolved Hide resolved
plugins/modules/system/sysupgrade.py Outdated Show resolved Hide resolved
@ansibullbot
Copy link
Collaborator

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

plugins/modules/system/sysupgrade.py:0:0: parameter-type-not-in-doc: Argument 'installurl' in argument_spec defines type as 'str' but documentation doesn't define type

click here for bot help

@ansibullbot ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label May 17, 2020
@ansibullbot ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label Jul 3, 2020
@felixfontein felixfontein changed the base branch from master to main July 6, 2020 07:56
@ansibullbot ansibullbot added the new_contributor Help guide this first time contributor label Jul 6, 2020
@precurse
Copy link
Contributor Author

Hey, sorry, I got caught up with some things and haven't been able to take a look at this for a while.

I see that @Akasurde made a commit in early-July for this module. Do you need any other modifications done to it?

Thanks!

@ansibullbot ansibullbot added affects_1 stale_ci CI is older than 7 days, rerun before merging and removed stale_ci CI is older than 7 days, rerun before merging labels Aug 16, 2020
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

I don't know sysupgrade, so only some general remarks from my side.

plugins/modules/system/sysupgrade.py Show resolved Hide resolved
plugins/modules/system/sysupgrade.py Show resolved Hide resolved
@ansibullbot
Copy link
Collaborator

The test ansible-test sanity --test pylint [explain] failed with 1 error:

tests/unit/plugins/modules/system/test_sysupgrade.py:17:12: undefined-variable: Undefined variable 'fail_json'

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

tests/unit/plugins/modules/system/test_sysupgrade.py:11:1: E302: expected 2 blank lines, found 1
tests/unit/plugins/modules/system/test_sysupgrade.py:19:1: E302: expected 2 blank lines, found 1

The test ansible-test sanity --test pylint [explain] failed with 1 error:

tests/unit/plugins/modules/system/test_sysupgrade.py:17:12: undefined-variable: Undefined variable 'fail_json'

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

tests/unit/plugins/modules/system/test_sysupgrade.py:11:1: E302: expected 2 blank lines, found 1
tests/unit/plugins/modules/system/test_sysupgrade.py:19:1: E302: expected 2 blank lines, found 1

The test ansible-test sanity --test pylint [explain] failed with 1 error:

tests/unit/plugins/modules/system/test_sysupgrade.py:17:12: undefined-variable: Undefined variable 'fail_json'

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

tests/unit/plugins/modules/system/test_sysupgrade.py:11:1: E302: expected 2 blank lines, found 1
tests/unit/plugins/modules/system/test_sysupgrade.py:19:1: E302: expected 2 blank lines, found 1

click here for bot help

@precurse precurse force-pushed the obsd_sysupgrade branch 2 times, most recently from 9ab776a to c70992c Compare August 16, 2020 20:55
Copy link
Contributor Author

@precurse precurse left a comment

Choose a reason for hiding this comment

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

Adding version_added field.
Adding note to fetch_only that describes reboot behavior.

@felixfontein
Copy link
Collaborator

@precurse in case you added these changes, you didn't push them.

@precurse
Copy link
Contributor Author

precurse commented Aug 17, 2020

@felixfontein Odd.. I wonder if Github got mixed up with my force push and your suggestions.

Either way, I just added and pushed them properly now :)

@ansibullbot
Copy link
Collaborator

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

plugins/modules/system/sysupgrade.py:37:161: E501: line too long (162 > 160 characters)

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

plugins/modules/system/sysupgrade.py:37:161: E501: line too long (162 > 160 characters)

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

plugins/modules/system/sysupgrade.py:37:161: E501: line too long (162 > 160 characters)

click here for bot help

@ansibullbot ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label Aug 17, 2020
* Adding types to sysupgrade documentation
* Apply suggestions from code review
* Adding installurl flag. Changing wording in example.
* Use None for installurl by default
* Changing word case in description
* sysupgrade: use module structure recommended by Ansible unit test docs
* Adding unit test for sysupgrade

Signed-off-by:  Andrew Klaus <andrew@aklaus.ca>
Co-authored-by: Abhijeet Kasurde <akasurde@redhat.com>
Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
Co-authored-by: Felix Fontein <felix@fontein.de>
@ansibullbot ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label Aug 17, 2020
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

LGTM

@Andersson007 @Akasurde any last comments? Otherwise I'll merge for the 1.1.0 release today.

Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

LGTM
@felixfontein , no questions
@precurse thanks for the feature!

@felixfontein felixfontein merged commit 2aabf5e into ansible-collections:main Aug 18, 2020
@felixfontein
Copy link
Collaborator

@precurse thanks a lot for doing this!
@Akasurde @Andersson007 thanks a lot for reviewing this!

felixfontein pushed a commit that referenced this pull request Aug 18, 2020
* Adding types to sysupgrade documentation
* Apply suggestions from code review
* Adding installurl flag. Changing wording in example.
* Use None for installurl by default
* Changing word case in description
* sysupgrade: use module structure recommended by Ansible unit test docs
* Adding unit test for sysupgrade

Signed-off-by:  Andrew Klaus <andrew@aklaus.ca>
Co-authored-by: Abhijeet Kasurde <akasurde@redhat.com>
Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
Co-authored-by: Felix Fontein <felix@fontein.de>

Co-authored-by: Abhijeet Kasurde <akasurde@redhat.com>
Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit 2aabf5e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has_issue module module needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_contributor Help guide this first time contributor new_module New module new_plugin New plugin tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants