Skip to content
This repository has been archived by the owner on Nov 19, 2021. It is now read-only.

Version 1.4.x #131

Closed
wants to merge 4 commits into from
Closed

Conversation

psakar
Copy link
Contributor

@psakar psakar commented Nov 9, 2018

Checklist:

  • Have you added a note in the CHANGELOG.md for your change if user-facing?
    no
  • Have you added unit tests for your change?
    no

Initial python 3.x compatiblity.
Requires configparser library change from #130

@thescouser89 thescouser89 self-requested a review November 9, 2018 15:57
@thescouser89
Copy link
Contributor

Adding "future" to our Jenkins nodes. Will retest afterwards

@thescouser89
Copy link
Contributor

retest this please

@janinko
Copy link
Contributor

janinko commented Nov 12, 2018

The changes in pnc_cli/swagger_client/api_client.py must also be done in templates

Copy link
Contributor

@janinko janinko left a comment

Choose a reason for hiding this comment

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

The changes in pnc_cli/swagger_client/api_client.py must also be done in templates

Also you can rebease it now as the other PR was merged.

from tasks import Tasks
# import utils
# from scm_utils import ScmInfo, get_scm_info
# from tasks import Tasks
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't comment out code, either keep it or remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! We're actually using all those imports. It should be commented out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right. mistake. when you will remove the comment, you must change the import to conform to python 2 and python 3 rules at the same time (ATM it works only in python 2 but not in 3) - eg. "import utils" -> "from . import utils"

@psakar
Copy link
Contributor Author

psakar commented Nov 14, 2018

@janinko Please note I have no time for the requested changes as I've indicated via email. May be instead of requesting changes doing them directly would be quicker ?

@thescouser89
Copy link
Contributor

I'll try to do the necessary adjustments

@thescouser89
Copy link
Contributor

At this point, I think the best course of action to resolve the failure to rebase due to conflicts is to cherry-pick Petr's commit on top of version-1.4.x branch, and apply some changes on top to get the PR more easily rebased. I'll create another PR for it on Thursday hopefully. I'm keeping it open for now so that I remember :D

@psakar
Copy link
Contributor Author

psakar commented Nov 15, 2018

@thescouser89 Big thanx for taking over !

@thescouser89
Copy link
Contributor

PR created here: #134

Will close this PR.

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

Successfully merging this pull request may close these issues.

None yet

3 participants