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

Fix gui job logs viewer submit num count #2803

Merged
merged 2 commits into from
Oct 17, 2018

Conversation

matthewrmshin
Copy link
Contributor

Reset submit number correctly on trigger --edit back out.

Fix #2629.

@matthewrmshin matthewrmshin added this to the soon milestone Oct 16, 2018
@matthewrmshin matthewrmshin self-assigned this Oct 16, 2018
@kinow
Copy link
Member

kinow commented Oct 16, 2018

Looks good to me, LGTM +1

Reproduced the issue with the latest version of master branch. Note that the directory was created even though I clicked No.

error

Checked out the fix from this pull request, and tested again. Note now that the contents of the folder are the same, which means a new folder was not created.

fix

@hjoliver
Copy link
Member

I'm seeing something slightly different to @kinow on current master:

  • "Trigger (edit run)" creates the new submit directory (02) and symlinks NN -> 02 - expected, as the new job script is generated in that directory
  • When I back out (choose not to submit the edited job script) - the 02 directory gets deleted, and the NN symlink gets pointed back to the 01 directory. - do you not see this Bruno??

However, the symlink changes from relative to abs path. Original:

NN -> 01

new:

NN -> /full/path/to/01

@hjoliver
Copy link
Member

(However, the GUI does still look for submit 02 by default - which is the topic of this PR).

@hjoliver
Copy link
Member

Agree that the GUI problem is fixed on this branch.

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

@matthewrmshin - I won't merge this as I don't think you had requested review yet. Go ahead and merge it yourself it is all done...

@hjoliver hjoliver modified the milestones: soon, next release Oct 16, 2018
@kinow
Copy link
Member

kinow commented Oct 17, 2018

Oh, @hjoliver yup, if I edit the task definition and press No, the symlink is altered to point to the full path.

@matthewrmshin I was feeling a bit braver and more confident, and decided to give it a try and see if I could review some pull request. Clearly I need to practice more my Cylc-Fu :-) so don't mind about my previous comment. I didn't completely test the issue.

But as I never give up... here's another try. Reproducing the issue in master

image

And then using the branch from this pull request, the GUI correctly displays the job output.

image

Reset submit number correctly on `trigger --edit` back out.
@matthewrmshin
Copy link
Contributor Author

Branch re-based. Added new logic in existing test to cover #2804 and this PR.

@matthewrmshin
Copy link
Contributor Author

@kinow @hjoliver Sorry, please review the new test as well.

@kinow
Copy link
Member

kinow commented Oct 17, 2018

@matthewrmshin looks good! I have to learn how to write these test for test-battery too.

Test code looks OK. Here's the output in my environment after running the test:

kinow@kinow-VirtualBox:~/Development/python/workspace/cylc$ cylc test-battery tests/cylc-trigger/07-edit-run-abort.t 
tests/cylc-trigger/07-edit-run-abort.t .. ok   
All tests successful.
Files=1, Tests=2, 14 wallclock secs ( 0.03 usr  0.01 sys +  1.92 cusr  0.43 csys =  2.39 CPU)
Result: PASS

LGTM +1

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

Re-approving, with new test. All good.

@hjoliver hjoliver merged commit 4b34bdc into cylc:master Oct 17, 2018
@matthewrmshin matthewrmshin deleted the fix-gui-job-log branch October 17, 2018 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants