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

spawner.py: upload a basic results summary page #24

Merged
merged 3 commits into from Feb 17, 2017
Merged

Conversation

jlebon
Copy link
Collaborator

@jlebon jlebon commented Feb 17, 2017

Setting the commit status URL to the branch itself when there are
testsuites marked required is not very intuitive. Additionally, the
commit can disappear from the branch, giving users no way to discover
the statuses.

We now instead link to a very simple summary page containing the links
to all the other test results. When redhat-ci is used in combination
with homu, this will be accessible directly from the PR through the
comment homu posts.

Closes: #23

Setting the commit status URL to the branch itself when there are
testsuites marked required is not very intuitive. Additionally, the
commit can disappear from the branch, giving users no way to discover
the statuses.

We now instead link to a very simple summary page containing the links
to all the other test results. When redhat-ci is used in combination
with homu, this will be accessible directly from the PR through the
comment homu posts.

Closes: #23
@jlebon
Copy link
Collaborator Author

jlebon commented Feb 17, 2017

@jlebon
Copy link
Collaborator Author

jlebon commented Feb 17, 2017

@ashcrow Can you give a quick sanity check on the Jinja2 stuff?

spawner.py Outdated
s3_key = '%s/%s/%s.%s/%s' % (os.environ['s3_prefix'],
os.environ['github_repo'],
os.environ['github_commit'],
random.randint(0, 2**15-1),
Copy link
Member

Choose a reason for hiding this comment

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

Hm, why a random number here instead of e.g. a timestamp or global counter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The random number (also done in the testrunner) is a cheap way to avoid overwriting results if the same commit gets tested again (it's useful to keep the old results not only for historical reasons, but also e.g. if you have independent runs of redhat-ci against the same commit). Though maybe it's time to do this the proper way (i.e. check if it already exists rather than hope for the best).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I understand why it's there, I mean why allow the possibility of overwriting? A timestamp would be more reliably unique.

Copy link
Member

Choose a reason for hiding this comment

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

That said it doesn't seem like a big deal, I'm fine with it as is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Meh, a bit overkill but I updated it to use date +%s%N.

@ashcrow
Copy link
Contributor

ashcrow commented Feb 17, 2017

I don't think you need the i18n extension since nothing is being translated in the template but, from a quick eye parse, the jinja2 stuff looks fine.

@ashcrow
Copy link
Contributor

ashcrow commented Feb 17, 2017

👍

@jlebon jlebon merged commit d739c39 into master Feb 17, 2017
@jlebon jlebon deleted the pr/required-results branch July 7, 2017 15:52
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