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 lack of errors/warnings for deprecated [runtime][<task>][remote]retrieve job logs * settings #4948

Merged
merged 6 commits into from
Jul 21, 2022

Conversation

MetRonnie
Copy link
Member

@MetRonnie MetRonnie commented Jun 29, 2022

These changes close #4944.

  • Setting [runtime][<task>][remote]retrieve job logs = false on a task specified in the old way results in a "No platform found matching your task" error (my platform definition for the matching host has log retrieval enabled) . This appears to working as intended.
  • Setting [runtime][<task>][remote]retrieve job logs = false on a task specified using a platform does not result in a validation error so this needs fixing.

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.cfg and conda-environment.yml.
  • Updated tests
  • Change log entry
  • No cylc-doc PR required.

@MetRonnie MetRonnie added small doc Documentation labels Jun 29, 2022
@MetRonnie MetRonnie added this to the cylc-8.0rc4 milestone Jun 29, 2022
@MetRonnie MetRonnie self-assigned this Jun 29, 2022
Comment on lines 1409 to 1410
Conf(
'retrieve job logs', VDR.V_BOOLEAN,
Copy link
Member Author

Choose a reason for hiding this comment

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

Why isn't there a default value for this (True or False)?

Copy link
Member

Choose a reason for hiding this comment

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

Because history I'm afraid. We should make these defaults explicit - #4847

@oliver-sanders oliver-sanders requested review from hjoliver and removed request for wxtim June 29, 2022 15:57
@hjoliver
Copy link
Member

(Review pending response to this: #4944 (comment) )

@MetRonnie MetRonnie marked this pull request as draft June 30, 2022 08:10
@hjoliver
Copy link
Member

hjoliver commented Jul 3, 2022

From @dpmatthews explanatory comment on #4944:

Setting [runtime][<task>][remote]retrieve job logs = false on a task specified using a platform does not result in a validation error so this needs fixing.

So I guess you can just fix that here @MetRonnie, and we're good to go.

@hjoliver hjoliver modified the milestones: cylc-8.0rc4, cylc-8.0.0 Jul 5, 2022
@MetRonnie MetRonnie changed the title Fix incorrect deprecation notice for [runtime][<task>][remote] Fix lack of errors/warnings for deprecated [runtime][<task>][remote]retrieve job logs * settings Jul 19, 2022
@MetRonnie MetRonnie added bug Something is wrong :( and removed doc Documentation labels Jul 19, 2022
@MetRonnie MetRonnie marked this pull request as ready for review July 19, 2022 15:16
@hjoliver
Copy link
Member

(Some functional test failures)

@MetRonnie
Copy link
Member Author

MetRonnie commented Jul 20, 2022

# There are 2 remote tasks. One with "retrieve job logs = True", one without.
# Only t1 should have job.err and job.out retrieved.

Not sure how to achieve this, I think a solution may be:

  • Do not deprecate flow.cylc[runtime][task][remote]retrieve job logs in favour of platforms
  • Instead, move it up a section
  • This will override the platform default
  • Otherwise, the retrieve job logs max size and retrieve job logs retry delays should be deprecated in favour of platforms

After discussing, solution is to check blame and either stop using platforms for this test or remove the test

@MetRonnie MetRonnie added the question Flag this as a question for the next Cylc project meeting. label Jul 20, 2022
@MetRonnie MetRonnie requested a review from wxtim July 20, 2022 13:00
@MetRonnie MetRonnie removed the question Flag this as a question for the next Cylc project meeting. label Jul 20, 2022
Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

  • Looks sane
  • Local testing suggests that it works

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.

👍

@hjoliver hjoliver merged commit 40868d2 into cylc:master Jul 21, 2022
@MetRonnie MetRonnie deleted the remote-section branch July 21, 2022 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :( small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[runtime][<task>][remote] whole section deprecation notice?
4 participants