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

[wip] Job id prefix (jid) is now using travis numbers, shoudl work for PRs … #279

Merged
merged 2 commits into from Nov 16, 2016

Conversation

ssbarnea
Copy link
Member

…too.

Change-Id: I17bb2e4ab0651edb6aad7b1cad0c9d65c7ebfb63
Signed-off-by: Sorin Sbarnea ssbarnea@redhat.com

@ssbarnea ssbarnea added the Status: In progress Currently being worked on. label Nov 12, 2016
@ssbarnea ssbarnea added this to the 1.0.8 milestone Nov 12, 2016
@ssbarnea ssbarnea self-assigned this Nov 12, 2016
@ssbarnea ssbarnea force-pushed the feature/travis branch 4 times, most recently from 3a19ec4 to 9004766 Compare November 12, 2016 16:30
…too.

Change-Id: I17bb2e4ab0651edb6aad7b1cad0c9d65c7ebfb63
Signed-off-by: Sorin Sbarnea <ssbarnea@redhat.com>
Worklog, IssueLink, IssueLinkType, IssueType, Priority, Version, Role, Resolution, SecurityLevel, Status, User, \
CustomFieldOption, RemoteLink
from jira.resources import * # NOQA
import xml.etree.ElementTree as etree
Copy link
Member

Choose a reason for hiding this comment

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

Please use defusedxml to access ElementTree instead of directly using it.

@ssbarnea ssbarnea force-pushed the feature/travis branch 10 times, most recently from 59d8ffc to ccf1291 Compare November 16, 2016 23:11
Change-Id: I4c069a777986452ea275417f7526dd24291f41a4
Signed-off-by: Sorin Sbarnea <ssbarnea@redhat.com>
Copy link
Contributor

@asqui asqui left a comment

Choose a reason for hiding this comment

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

Just trying to grok some of the test changes...

getpass.getuser().upper()))[0:6] + \
str(sys.version_info[0]) + \
str(sys.version_info[1])
""" `jid` is important for avoiding concurency problems when
Copy link
Contributor

@asqui asqui Nov 17, 2016

Choose a reason for hiding this comment

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

Looks like the comments above no longer reflect what the code is doing, which is not helpful :-(

tests outside Travis
* Travis is using "Travis" username

https://docs.travis-ci.com/user/environment-variables/
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused. That page says not to depend on the USER being travis...

if user == 'TRAVIS' and 'TRAVIS_JOB_NUMBER' in os.environ:
# please note that user underline (_) is not suppored by
# jira even if is documented as supported.
self.jid = 'T' + hashify(user + os.environ['TRAVIS_JOB_NUMBER'])
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we don't need to bring travis user into it if we're namespacing the jid with T for travis and Z for others?

Also, TRAVIS_JOB_NUMBER is going to be something like 531.3 - why do we even need to hash this to 8 digits? Can't we just replace the '.' and have a much more human-readable project key?

identifier = user + \
chr(ord('A') + sys.version_info[0]) + \
chr(ord('A') + sys.version_info[1])
self.jid = 'Z' + hashify(identifier)
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, do we really need to hash this? With the old ZUSERNAME35 the only way we'll clash is if two users that start with the same 8 letters are running tests simultaneously (on the same version of python). Do we have any evidence of this being a problem?

And even if it was, it would never affect Travis because of the Z prefix, which is the important part...

ssbarnea added a commit that referenced this pull request Nov 21, 2016
Job id prefix (jid) is now using travis numbers, should work for PRs …
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: In progress Currently being worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants