Make into a package #41
Conversation
There was a problem hiding this comment.
Looks generally OK apart from the one comment about overlib.
About testing, can you set this up in a dedicated testing Ska3 and run it in parallel for a week or two (maybe without the --email flag)? I'm not sure how hard this would be out of the box, but we do need to be very confident that it will work the first time when we promote.
|
With regard to testing, I don't quite see the need for a soak period? This doesn't really have different behaviors over time. |
jobwatch/jobwatch.py
Outdated
| @@ -35,7 +35,7 @@ def __init__(self, task, filename, | |||
| self.maxage = maxage | |||
| self.filetime = None | |||
| self.filedate = None | |||
|
|
|||
| self.type = 'Job' | |||
There was a problem hiding this comment.
This hack was to make a test pass, but I think I should probably just update the code in reporting that looks for a "jw.type" and skip if type is missing.
There was a problem hiding this comment.
Does it work if you assign None. This would be a bit more agnostic.
There was a problem hiding this comment.
No, None didn't work. And setting it to 'Job' seemed to just override the rest of the types. I didn't have the brainpower just now to figure out the inheritance and just updated the logic in the html report method for now.
|
The soak period will naturally happen between when you start the job going and we are ready with a release. I'd just like to see an end-to-end test for a few days because it is so easy to make a little mistake in packaging (I've done this many times). |
|
@taldcroft if you were interested in installing this in the next ska3 update but also running on the side, I think we would have a little bit of a collision issue for data/skawatch3 and www/skawatch3 . Did you want to, say, reclaim |
|
Actually, since we win nothing (no updated checks or fixes in here) by putting this PR in skare3 2021.1, I think we should just hold this one. |
| this_type = getattr(jw, 'type', 'Job') | ||
| last_type = getattr(jobwatches[i_jw -1], 'type', 'Job') | ||
| if i_jw == 0 or this_type != last_type: | ||
| jw.span_cols_text = this_type |
There was a problem hiding this comment.
I'm not sure if jw type should be a property, but this more explicit handling of the attr at least seemed not to break much (related to comment at #41 (comment)) .
|
OK. I think I've addressed the comments and we probably don't care about fixing jobwatch type inheritance or whatever, so merging despite the "changes requested" bit. |
Make into a package
Not sure what testing would be appropriate, but there's a change in here so it at least passes all of its own tests.