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

Fortios file only mode + integration tests #23275

Merged
merged 18 commits into from May 9, 2017

Conversation

bjolivot
Copy link
Contributor

@bjolivot bjolivot commented Apr 4, 2017

SUMMARY

As Fortigate device could not be buy for only ansible tests, I have added file mode in fortios module_utils helper.
File mode mean that module work with only configuration file level, without device needed \o/.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

module_utils/fortios.py

ANSIBLE VERSION
ansible 2.4.0 (fortios_file_only_mode c8cafae84a) last updated 2017/04/04 23:18:30 (GMT +200)
ADDITIONAL INFORMATION

@ansibot ansibot added affects_2.4 This issue/PR affects Ansible v2.4 c:module_utils/ community_review In order to be merged, this PR must follow the community review workflow. feature_pull_request module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. networking Network category labels Apr 4, 2017
@ansibot
Copy link
Contributor

ansibot commented Apr 4, 2017

The test ansible-test sanity --test integration-aliases failed with the following error:

Command "test/sanity/code-smell/integration-aliases.py" returned exit status 1.
>>> Standard Output
The following integration target directories are missing `aliases` files:

test/integration/targets/fortios_ipv4_policy

If these tests cannot be run as part of CI (requires external services, unsupported dependencies, etc.),
then they most likely belong in `test/integration/roles/` instead of `test/integration/targets/`.
In that case, do not add an `aliases` file. Instead, just relocate the tests.

However, if you think that the tests should be able to be supported by CI, please discuss test
organization with @mattclay or @gundalow on GitHub or #ansible-devel on IRC.

If these tests can be run as part of CI, you'll need to add an appropriate CI alias, such as:

posix/ci/group1
windows/ci/group2

The CI groups are used to balance tests across multiple jobs to minimize test run time.
Using the relevant `group1` entry is fine in most cases. Groups can be changed later to redistribute tests.

Aliases can also be used to express test requirements:

needs/privileged
needs/root
needs/ssh

Other aliases are used to skip tests under certain conditions:

skip/freebsd
skip/osx
skip/python3

Take a look at existing `aliases` files to see what aliases are available and how they're used.

The test ansible-test sanity --test yamllint failed with the following errors:

test/integration/targets/fortios_ipv4_policy/tasks/test_indempotency.yml:69:1: too many blank lines (1 > 0) (empty-lines)
test/integration/targets/fortios_ipv4_policy/tasks/test_params.yml:76:1: too many blank lines (1 > 0) (empty-lines)

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Apr 4, 2017
@ansibot
Copy link
Contributor

ansibot commented Apr 5, 2017

The test ansible-test sanity --test integration-aliases failed with the following error:

Command "test/sanity/code-smell/integration-aliases.py" returned exit status 1.
>>> Standard Output
The following integration target directories are missing `aliases` files:

test/integration/targets/fortios_ipv4_policy

If these tests cannot be run as part of CI (requires external services, unsupported dependencies, etc.),
then they most likely belong in `test/integration/roles/` instead of `test/integration/targets/`.
In that case, do not add an `aliases` file. Instead, just relocate the tests.

However, if you think that the tests should be able to be supported by CI, please discuss test
organization with @mattclay or @gundalow on GitHub or #ansible-devel on IRC.

If these tests can be run as part of CI, you'll need to add an appropriate CI alias, such as:

posix/ci/group1
windows/ci/group2

The CI groups are used to balance tests across multiple jobs to minimize test run time.
Using the relevant `group1` entry is fine in most cases. Groups can be changed later to redistribute tests.

Aliases can also be used to express test requirements:

needs/privileged
needs/root
needs/ssh

Other aliases are used to skip tests under certain conditions:

skip/freebsd
skip/osx
skip/python3

Take a look at existing `aliases` files to see what aliases are available and how they're used.

click here for bot help

@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Apr 5, 2017
@gundalow
Copy link
Contributor

gundalow commented Apr 5, 2017

I see you have the aliases set now, which looks good.
I've been busy with 2.3 stuff, so apologies for not having time on IRC, we can have the discussion here.

Could you please rebase your PR

@gundalow gundalow removed the needs_triage Needs a first human triage before being processed. label Apr 5, 2017
@ansibot
Copy link
Contributor

ansibot commented Apr 6, 2017

@bjolivot this PR contains the following merge comits:

Please rebase your branch to remove these commits.

click here for bot help

1 similar comment
@ansibot
Copy link
Contributor

ansibot commented Apr 6, 2017

@bjolivot this PR contains the following merge comits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibot ansibot added the merge_commit This PR contains at least one merge commit. Please resolve! label Apr 6, 2017
@mattclay
Copy link
Member

mattclay commented Apr 7, 2017

CI failure in integration tests on centos6:

https://app.shippable.com/github/ansible/ansible/runs/18428/12/console

The error was: Could not import the python library pyFG required by this module

@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Apr 7, 2017
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Apr 10, 2017
@bjolivot
Copy link
Contributor Author

ready_for_review

@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Apr 10, 2017
@@ -0,0 +1,9 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need this file as ansible-test will create one, also it could get out of sync with the actual tests.

ansible-test integration -vvve fortios.*

The -e means explain, notice that a Playbook is generated on the fly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

id: 42
src_addr: all
dst_addr: all
# policy_action: accept
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this comment about, left over from development?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not left over.
I assert that missing required param thow an exception, for all these test, I comment the param line.

@gundalow
Copy link
Contributor

From a quick review it looks good. Will review in more detail when I get a chance.

A number of files appear to be missing newlines, could you please fix that, thanks.

@bjolivot
Copy link
Contributor Author

ready_for_review

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Apr 28, 2017
@bjolivot
Copy link
Contributor Author

bjolivot commented May 5, 2017

bot_status

@ansibot
Copy link
Contributor

ansibot commented May 5, 2017

waiting_on: ansible
component: None
supported_by: core
changes_requested_by: null
needs_info: False
needs_revision: False
needs_rebase: False
merge_commits: []
mergeable_state: clean
shippable_status: success
maintainer_shipits: False
community_shipits: False
ansible_shipits: False
shipit_actors: None

click here for bot help

@gundalow gundalow closed this May 9, 2017
@gundalow gundalow reopened this May 9, 2017
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label May 9, 2017
@gundalow gundalow merged commit e99815e into ansible:devel May 9, 2017
dpiotrowski pushed a commit to dpiotrowski/ansible that referenced this pull request May 9, 2017
* WIP file_mode

* WIP

* Add file_mode + integration tests

* fix pep8

* Update doc fragments
Create mutualy_exclusive param
Fix yamllint problem in tests

* Add aliases file + main playbook for fortios

* Install pyfg before running tests

* Install pyfg before running tests in role

* Remove pre_task as it's done in roles

* Force pyFG minimal version for python3

* role_path not role_dir :(

* Change requirements

* Specify Error type when error on import

* Bug in pygf library with python 2.5 (PR is waiting spotify/pyfg#19)

* Bad requirement format

* still bad format -_-'

* remove test/integration/fortios.py (auto generated by tests)
missing new lines at end of file

* pyFG is now fixed in 0.50
KKoukiou pushed a commit to KKoukiou/ansible that referenced this pull request May 22, 2017
* WIP file_mode

* WIP

* Add file_mode + integration tests

* fix pep8

* Update doc fragments
Create mutualy_exclusive param
Fix yamllint problem in tests

* Add aliases file + main playbook for fortios

* Install pyfg before running tests

* Install pyfg before running tests in role

* Remove pre_task as it's done in roles

* Force pyFG minimal version for python3

* role_path not role_dir :(

* Change requirements

* Specify Error type when error on import

* Bug in pygf library with python 2.5 (PR is waiting spotify/pyfg#19)

* Bad requirement format

* still bad format -_-'

* remove test/integration/fortios.py (auto generated by tests)
missing new lines at end of file

* pyFG is now fixed in 0.50
@ansibot ansibot added feature This issue/PR relates to a feature request. and removed feature_pull_request labels Mar 4, 2018
@dagwieers dagwieers added the fortios Fortios community label Feb 22, 2019
@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.4 This issue/PR affects Ansible v2.4 c:module_utils/ feature This issue/PR relates to a feature request. fortios Fortios community networking Network category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants