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

Fix CI and drop support for py35 #1015

Closed
wants to merge 27 commits into from
Closed

Fix CI and drop support for py35 #1015

wants to merge 27 commits into from

Conversation

adehad
Copy link
Contributor

@adehad adehad commented Apr 24, 2021

Making a MR to view the CI after trying out some changes based on the discussions in #896

Updated the interfaces to match "Jira Server" v2 api - what the docker image is based off (vs "Cloud").
Referring to : https://docs.atlassian.com/software/jira/docs/api/REST/8.13.6/#api/2/

FYI: @Addono @ssbarnea - I'd love to drop Py35 support and use cleaner f-strings everywhere, I can imagine these changes would be a major bump anyway due to the using v2 endpoints - so might be worth doing at the same time, but perhaps a separate pull request.

studioj
studioj previously approved these changes Apr 24, 2021
Copy link
Collaborator

@studioj studioj left a comment

Choose a reason for hiding this comment

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

small remark => should it go to master or first to fix ci?

Edit:
dammit first time reviewer in a larger github project :-s the approval was ment for the first commit only. And i cant really find a way to "un approve"

I'll try to complete the review asap

Edit2: code review done => see below, it looks promising but I'll need to run locally to fully understand and validate :-)

a quick glance at the CI system shows some missing stuff still

https://travis-ci.com/github/pycontribs/jira/jobs/500924272

If i can collaborate here let me know (it's quite late here atm but i'll try to have a look tomorrow )

Copy link
Collaborator

@studioj studioj left a comment

Choose a reason for hiding this comment

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

looks legit 👍

I'll check out yr branch later today or tomorrow and try to run it locally!

btw thanks a lot for the windows run. I'll try that for sure 👍

@@ -0,0 +1,16 @@
from jira import JIRA
Copy link
Collaborator

@studioj studioj Apr 24, 2021

Choose a reason for hiding this comment

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

nice i wash just cleaning up that functionality too 👍
maybe this can be used in the test.local script too

jira/client.py Outdated
)

@property
def latest_rest(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

cleaner, but i need to mention you're moving something private to the public space.

just need to think about it => more public api => more stuff to maintain :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep moved to a private method based of the existing _get_url()

jira/client.py Outdated
@@ -798,7 +821,7 @@ def set_application_property(self, key, value):
:param value: value to assign to the property
:type value: str
"""
url = self._options["server"] + "/rest/api/latest/application-properties/" + key
url = self.latest_rest + "/application-properties/" + key
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a lot clearer 👍

@@ -3765,7 +3767,8 @@ def create_project(
"key": key,
"projectTypeKey": ptype,
"projectTemplateKey": template_key,
"leadAccountId": assignee,
"lead": assignee,
# "leadAccountId": assignee,
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this remain here? => i'd say remove the comment if it fixes it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, there's probably a bigger conversation on how we handle Jira Cloud vs Jira Datacenter/Server, but will remove these for now

test.local Outdated
until $(curl -u $CI_JIRA_ADMIN:$CI_JIRA_ADMIN_PASSWORD --output /dev/null --silent --head --fail $CI_JIRA_URL/rest/api/2/permissions); do end_time="$(date -u +%s)";elapsed="$(($end_time-$start_time))"; echo "not running yet after $elapsed seconds $CI_JIRA_URL";sleep 5; done
pip install -e .
echo adding user $CI_JIRA_USER to $CI_JIRA_URL
(python -c "from jira import JIRA;jira_client = JIRA('$CI_JIRA_URL', basic_auth=('$CI_JIRA_ADMIN', '$CI_JIRA_ADMIN_PASSWORD'));a = jira_client.add_user('$CI_JIRA_USER', 'user@example.com', password='$CI_JIRA_USER_PASSWORD') if not jira_client.user('$CI_JIRA_USER') else None" && echo "Created user '$CI_JIRA_USER', or user was already present") || (echo "Failed creating user '$CI_JIRA_USER'" && docker logs --tail 500 jira)
Copy link
Collaborator

@studioj studioj Apr 24, 2021

Choose a reason for hiding this comment

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

maybe your make_local_jira_user.py script can be called here

@adehad
Copy link
Contributor Author

adehad commented Apr 24, 2021

small remark => should it go to master or first to fix ci?

@studioj I think it can go to master as I've merged the fix_ci stuff already into this branch.

a quick glance at the CI system shows some missing stuff still

https://travis-ci.com/github/pycontribs/jira/jobs/500924272

If i can collaborate here let me know (it's quite late here atm but i'll try to have a look tomorrow )

@studioj definitely appreciate the reviews ! I've just made a quick and dirty fix of all the tests. For whatever reason I cannot find jirashell and that test is the only test that fails locally for me.

Likewise a bit late on my side, so will give the reviews a look tomorrow, and tidy up some of the rogue comments and inconsistencies.

I've added you as a collaborator on my fork, so feel free to push stuff to there !

@adehad adehad marked this pull request as ready for review April 25, 2021 13:48
@adehad adehad changed the title WIP: attempting to fix CI fixing CI Apr 25, 2021
@adehad
Copy link
Contributor Author

adehad commented Apr 25, 2021

@ssbarnea looks like Travis isn't running any more builds on this PR, any chance you can manually trigger one?

@ssbarnea
Copy link
Member

Replace travis with gha, travis dead.

@studioj
Copy link
Collaborator

studioj commented Apr 25, 2021

local run
image

@studioj
Copy link
Collaborator

studioj commented Apr 25, 2021

@adehad want me to help in some part?, can i take something up?

@adehad
Copy link
Contributor Author

adehad commented Apr 25, 2021

@adehad want me to help in some part?, can i take something up?

@studioj Would be great if you are familiar with Github actions and can set something up. I'm not familiar at all so probably won't get very far today

@studioj
Copy link
Collaborator

studioj commented Apr 25, 2021

haha, if some experience is beter than none then I can try some stuff :-) => i've done one package already on github actions but compared to this one its pretty basic :-)
I'll give it a go
@ssbarnea I'll try to cover as much as the travis run

@studioj
Copy link
Collaborator

studioj commented Apr 25, 2021

i dont have permissions I'll fork from mine but i see you're busy too ...

@adehad
Copy link
Contributor Author

adehad commented Apr 25, 2021

i dont have permissions I'll fork from mine but i see you're busy too ...

hmm, I sent an invite that may have gone to your mail about giving you access, but feel free to fork off mine

@studioj
Copy link
Collaborator

studioj commented Apr 26, 2021

I'd love to drop Py35 support and use cleaner f-strings everywhere, I can imagine these changes would be a major bump anyway due to the using v2 endpoints - so might be worth doing at the same time, but perhaps a separate pull request.

@adehad beter to keep that for a separate PR.

Let's try to focus in this pr on CI improvements only. It will be large enough already :-)

@studioj
Copy link
Collaborator

studioj commented May 1, 2021

Ok CI is green in this branch.

see here: https://github.com/adehad/jira/actions/runs/803175998

We removed 3.5 support

  • that prevents us from locking half of the required packages.

most of the heavy lifting was done by @adehad I'm not that knowledgeable about tox (now a little more)
but we got the Jira Server CI tests up and running AND moved to GitHub Actions.

@ssbarnea I think security wise it might be possible to re add the credentials for testing JiraCloud in GitHub Actions itself.

I don't know if the CI system/test coverage is trustworthy in itself, I think @ssbarnea you can better asses that.

Eager to do some more, but this is a start, ready for getting some green back into this repo ;-)

@studioj
Copy link
Collaborator

studioj commented May 1, 2021

I would add some more intelligent static code analyzer in the CI system too... is there a preference? sourcery/LGTM/Sonar

@ssbarnea ssbarnea changed the title fixing CI Fix CI and drop support for py35 May 2, 2021
@ssbarnea ssbarnea added the bug label May 2, 2021
@ssbarnea
Copy link
Member

ssbarnea commented May 2, 2021

I fixed resolve conflicts in another branch and merged it. I will close this one now.

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

Successfully merging this pull request may close these issues.

4 participants