Fix suite report #158
Fix suite report #158
Conversation
james-bruten-mo
left a comment
james-bruten-mo
left a comment
There was a problem hiding this comment.
Hi Pierre, looks good so far, thanks. I've made a couple of suggestions - one I think we should keep the existing variable as it is. The other I've had a thought of how it should appear while doing the review.
I also think the "Suite Name" row at line 121 of suite_report_git.py should be edited to be a markdown formatted hyperlink, with text of self.workflow_id and a url of self.cylc_url.
The other thing is testing. I'm assuming you've run it from the command line. It'd also be good to do it from rose-stem (the trac.log you've pasted is using the existing version). In your UM clone you'll need to edit the entry for SimSys_Scripts to point at your fork and branch. Then if you run cylc vip -z g=check_groups_coverage -n test_suite_report ./rose-stem that'll run really quickly (no need to wait for developer)
github_scripts/suite_report_git.py
Outdated
| # Create Collapsed task tables | ||
| for state in order: | ||
| if state == "succeeded": | ||
| continue |
There was a problem hiding this comment.
This definitely works for what I initially suggested. But now I think about it, can we get so that it prints the number of succeeded tasks, without the dropdown table. I think this if statement needs to go to line 190, and then it should include a self.trac_log.append(f"{emoji} {state} tasks - {len(tasks)}") statement.
github_scripts/suite_data.py
Outdated
| except IndexError: | ||
| continue | ||
|
|
||
| suite_user = os.environ["USER"] |
There was a problem hiding this comment.
This section looks good. But can we not overwrite the workflow_id variable. Create a new function here to generate the url and then call self.cylc_url: str = self.generate_cylc_url() at line 76 of suite_report_git.py.
james-bruten-mo
left a comment
james-bruten-mo
left a comment
There was a problem hiding this comment.
Thanks, mostly there.
Could you add a trac.log produced by the updated script please. Grab me if you're unsure how to get one
github_scripts/suite_data.py
Outdated
| try: | ||
| workflow_id = match.group(1) | ||
| return workflow_id | ||
| break |
There was a problem hiding this comment.
Can you revert all changes to this function please - we want to still give the "unknown_workflow_id" result when we can't find it
github_scripts/suite_data.py
Outdated
| workflow_id = self.get_workflow_id() | ||
| suite_user = os.environ["USER"] | ||
|
|
||
| cylc_review = f"""[{workflow_id}](https://cylchub/services/cylc-review/cycles |
There was a problem hiding this comment.
This doesn't quite work - it will contain a newline character. If you do the following syntax, it will concatenate the strings. Note, there are no commas - if you add a comma it will turn it into a tuple
cylc_review = (
f"[{workflow_id}](https://cylchub/services/cylc-review/cycles"
f"/{suite_user}/?suite={workflow_id.replace("/","%2F")})"
)
github_scripts/suite_report_git.py
Outdated
| tasks = parsed_tasks[state] | ||
| if state == "succeeded": | ||
| continue | ||
| self.trac_log.append(f"{emoji} {state} tasks - {len(tasks)}") |
There was a problem hiding this comment.
We only want this line for the succeeded tasks so can you add in the if statement above the continue statement
github_scripts/suite_data.py
Outdated
| """ | ||
| Generate a markdown url to the cylc review page of a workflow | ||
| """ | ||
| workflow_id = self.get_workflow_id() |
There was a problem hiding this comment.
No need to rerun this function. By this point in the script self.workflow_id is set so we can just use that
This reverts commit 554e073.
james-bruten-mo
left a comment
james-bruten-mo
left a comment
There was a problem hiding this comment.
Perfect, thanks very much Pierre
2 participants
Description
Fixing suite_report_git.py
Summary
Currently suite_report_git.py does not provide a link to the cylc review page for a suite name and reports all succeeding tasks to make the suite report shorter to read, only failing tasks need to be reported additionally it would be useful for code reviewers if a link to the cylc review page for a suite run was provided as part of the report to enable clearer feedback through easier diagnosis of failing tasks.
Changes
This PR will change the formatting of the suite report
Dependency
N/A
Impact
N/A
Issues addressed
Resolves #156 and resolves #157
Coordinated merge
N/A
Checklist
Trac.log
Test Suite Results - um - test_report/run1
Suite Information
Approvals
Code Owners
Config Owners
No UM Config Owners Required
Task Information
✅ succeeded tasks - 5