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

Added search function to jira module and bugfixes #22

Merged
merged 32 commits into from
Apr 9, 2020

Conversation

pertoft
Copy link
Contributor

@pertoft pertoft commented Mar 19, 2020

SUMMARY

Added Jira JQL Search support updated docs and error handling

ISSUE TYPE
  • Bugfix Pull Request
  • Docs Pull Request
  • Feature Pull Request
COMPONENT NAME

modules/web_infrastructure/jira.py

ADDITIONAL INFORMATION
(venv) pto:ansible pto$ ansible-playbook jira-test.yml
[WARNING]: You are running the development version of Ansible. You should only run Ansible from "devel" if you are modifying the Ansible engine, or trying out features under development. This is a rapidly changing source of code and can become unstable at any point.
[WARNING]: No inventory was parsed, only implicit localhost is available
[WARNING]: provided hosts list is empty, only localhost is available. Note that the implicit localhost does not match 'all'

PLAY [localhost] ***********************************************************************************************************************************************************************************************************************************************************************************

TASK [Get CMDB issue] ******************************************************************************************************************************************************************************************************************************************************************************
changed: [localhost -> localhost]

TASK [debug] ***************************************************************************************************************************************************************************************************************************************************************************************
ok: [localhost] => {
    "cmdb_issue": {
        "changed": true,
        "failed": false,
        "meta": {
            "issues": [],
            "maxResults": 10,
            "startAt": 0,
            "total": 0
        }
    }
}

PLAY RECAP *****************************************************************************************************************************************************************************************************************************************************************************************
localhost                  : ok=2    changed=1    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0

(venv) pto:ansible pto$

plugins/modules/web_infrastructure/jira.py Outdated Show resolved Hide resolved
plugins/modules/web_infrastructure/jira.py Outdated Show resolved Hide resolved
plugins/modules/web_infrastructure/jira.py Outdated Show resolved Hide resolved
plugins/modules/web_infrastructure/jira.py Outdated Show resolved Hide resolved
plugins/modules/web_infrastructure/jira.py Outdated Show resolved Hide resolved
plugins/modules/web_infrastructure/jira.py Outdated Show resolved Hide resolved
@gundalow
Copy link
Contributor

Hi @Slezhuk and @tarka :)
Would you possibly be able to review this updated from @pertoft which adds search and update functionality to the jira Ansible module? Thanks in advance.

pertoft and others added 6 commits March 23, 2020 19:30
Co-Authored-By: John R Barker <john@johnrbarker.com>
Co-Authored-By: John R Barker <john@johnrbarker.com>
Co-Authored-By: John R Barker <john@johnrbarker.com>
Co-Authored-By: John R Barker <john@johnrbarker.com>
Co-Authored-By: John R Barker <john@johnrbarker.com>
Co-Authored-By: John R Barker <john@johnrbarker.com>
@felixfontein
Copy link
Collaborator

Please add a changelog fragment in changelogs/fragments/.

(You probably have to rebase with current master to see that directory. You can also simply create it yourself without rebasing; git does not care about directories, only about files.)

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.

thanks!

plugins/modules/web_infrastructure/jira.py Outdated Show resolved Hide resolved
plugins/modules/web_infrastructure/jira.py Show resolved Hide resolved
plugins/modules/web_infrastructure/jira.py Outdated Show resolved Hide resolved
plugins/modules/web_infrastructure/jira.py Outdated Show resolved Hide resolved
plugins/modules/web_infrastructure/jira.py Outdated Show resolved Hide resolved
@Andersson007
Copy link
Contributor

cc @Slezhuk @tarka

@Andersson007
Copy link
Contributor

copy of ansible/ansible#68336

pertoft and others added 3 commits April 3, 2020 12:34
Co-Authored-By: Andrew Klychkov <aaklychkov@mail.ru>
Co-Authored-By: Andrew Klychkov <aaklychkov@mail.ru>
Co-Authored-By: Andrew Klychkov <aaklychkov@mail.ru>
Copy link
Contributor Author

@pertoft pertoft left a comment

Choose a reason for hiding this comment

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

I have updated with your suggestions

@Andersson007
Copy link
Contributor

I've just written to atlassian about the changes (because the module was initially added by their employee, regarding to his mail address). Hope they'll give us somebody to take a look (or anybody else from the community will test it manually, it would be ok).
also we need a changelog fragment as @felixfontein mentioned above (the dir is community/general/changelogs)

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.

and please test url = url + '&fields=' + '&fields='.join([urllib.request.pathname2url(f) for f in fields])
manually, i'm not 100% sure it works as you expect

plugins/modules/web_infrastructure/jira.py Outdated Show resolved Hide resolved
@pertoft pertoft changed the title Added search function to jira module Added search function to jira module and bugfixes Apr 3, 2020
Co-Authored-By: Andrew Klychkov <aaklychkov@mail.ru>
@Andersson007
Copy link
Contributor

@pertoft

  1. what about @simonbaird 's suggestion?
  2. we need a person who'll check the module with these changes manually and confirm that all parameters work ok

@simonbaird
Copy link
Contributor

Should this be rebased and squashed into a few tidy commits, or will it all be squashed down on merge?

@Andersson007
Copy link
Contributor

@simonbaird on merge

@Andersson007
Copy link
Contributor

@pertoft please remove version_added. it'll live separatly from Ansible core. There's no versioning policy in community.general yet AFAIK

pertoft and others added 2 commits April 8, 2020 16:09
@pertoft
Copy link
Contributor Author

pertoft commented Apr 8, 2020

@pertoft please remove version_added. it'll live separatly from Ansible core. There's no versioning policy in community.general yet AFAIK

Done

@Andersson007
Copy link
Contributor

Andersson007 commented Apr 8, 2020

@simonbaird are you a jira user? If yes, could you please check the module with these changes manually and confirm that all parameters work ok.
(there are no ci tests for the module)

@simonbaird
Copy link
Contributor

Yeah okay, let me give it a try.

@simonbaird
Copy link
Contributor

simonbaird commented Apr 8, 2020

Regarding the maxRequests param, I'd be happy to add that later in a future MR.

Edit: Nevermind, I see it's added already.

@simonbaird
Copy link
Contributor

I've given search operation a quick test and it appears to work nicely. The maxresults param and the args/fields seem to work as expected.

@simonbaird
Copy link
Contributor

For the error message handling change, here's a before/after for an error message returned using search:

fatal: [localhost]: FAILED! => {"changed": false, "msg": "{\"errorMessages\":[\"The value 'Testingz' does not exist for the field 'status'.\"],\"errors\":{}}"}
fatal: [localhost]: FAILED! => {"changed": false, "msg": ["The value 'Testingz' does not exist for the field 'status'."]}

So it's an improvement.

Looking at that code, it might not work well if the response is valid json but the errorMessages key is not present, but I'm not sure if that's worth worrying about.

@Andersson007 Andersson007 merged commit 9d1880f into ansible-collections:master Apr 9, 2020
@Andersson007
Copy link
Contributor

merged into master

@Andersson007
Copy link
Contributor

@pertoft thanks for your work!
@simonbaird thanks for reviewing and testing!

@Andersson007
Copy link
Contributor

@gundalow @felixfontein thanks for reviewing!

@pertoft
Copy link
Contributor Author

pertoft commented Apr 9, 2020

Thank you all for helping! My first contribution 🤗
Error handling could use some extra work, but jira spits out different error variables and I don’t know which to filter for. Think it’s better to get too much than miss something. Will this module be updated in Ansible 2.9 or when will 2.10 be released?

@Andersson007
Copy link
Contributor

Definitely not in 2.9. Maybe in 2.10 but not sure.
@gundalow ?

@simonbaird
Copy link
Contributor

Awesome! FYI, I'm planning to use this right away to produce issue lists based on fixVersion.

@gundalow
Copy link
Contributor

Thank you all for helping! My first contribution
@pertoft woot, congratulations on your first PR being merged. I know things can be a little confusing at the moment as we build up the contributor workflow around Collections, so thank you for your understanding.

Error handling could use some extra work, but jira spits out different error variables and I don’t know which to filter for. Think it’s better to get too much than miss something. Will this module be updated in Ansible 2.9 or when will 2.10 be released?

This PR appears to add a new feature. We never backport new features to stable releases.

So you'll need to wait for Ansible 2.10 to be released.

@ktdreyer
Copy link
Contributor

ktdreyer commented May 8, 2020

I updated the error handling in #311

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants