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 shellcheck error SC2317 & failing test #2845

Merged
merged 5 commits into from
Jan 8, 2025

Conversation

wxtim
Copy link
Contributor

@wxtim wxtim commented Jan 8, 2025

No description provided.

@wxtim wxtim self-assigned this Jan 8, 2025
@wxtim wxtim added this to the 2.4.0 milestone Jan 8, 2025
@oliver-sanders
Copy link
Member

Seems like a reasonable check: https://www.shellcheck.net/wiki/SC2317

What's the bizz?

@wxtim wxtim changed the base branch from prepare-2.4.0 to master January 8, 2025 14:52
@wxtim
Copy link
Contributor Author

wxtim commented Jan 8, 2025

Seems like a reasonable check: https://www.shellcheck.net/wiki/SC2317

What's the bizz?

I don't want to change rose-mpi-launch - and the case statement looks valid: It's metomi/rose/scripts/rose-mpi-launch and it looks like it's the end of each --help case statement. It seems legit - but I'm not at all clear whether it is given the case statement context.

@MetRonnie
Copy link
Contributor

MetRonnie commented Jan 8, 2025

The shellcheck lint should be addressed like in cylc/cylc-flow#6408. But happy to punt to an issue for after the release

@oliver-sanders
Copy link
Member

I don't want to change rose-mpi-launch - and the case statement looks valid

We can ignore the one particular instance rather than ignoring the entire code globally.

@MetRonnie
Copy link
Contributor

1 test failing: t/rose-task-run/06-app-prune-iso.t

@oliver-sanders
Copy link
Member

the case statement looks valid:

I think it's the colon character that shellcheck is objecting to, and I think that's a valid objection:

https://stackoverflow.com/a/3224910

@wxtim
Copy link
Contributor Author

wxtim commented Jan 8, 2025

the case statement looks valid:

I think it's the colon character that shellcheck is objecting to, and I think that's a valid objection:

https://stackoverflow.com/a/3224910

Came to this conclusion independently. Just pushing.

@oliver-sanders
Copy link
Member

oliver-sanders commented Jan 8, 2025

1 test failing: t/rose-task-run/06-app-prune-iso.t

confirmed locally:

cylc.flow.exceptions.GraphParseError: Opposite outputs my_task_1:succeeded and my_task_1:failed must both be optional if both are used (via family trigger defaults

Easy fix.

@oliver-sanders
Copy link
Member

This'll do it:

diff --git a/t/rose-task-run/06-app-prune-iso/flow.cylc b/t/rose-task-run/06-app-prune-iso/flow.cylc
index d720a8dd0..f9abceb49 100644
--- a/t/rose-task-run/06-app-prune-iso/flow.cylc
+++ b/t/rose-task-run/06-app-prune-iso/flow.cylc
@@ -16,7 +16,7 @@
 {% if HOST != 'localhost' %}
                 my_task_2
 {% endif %}
-                WARM[-PT12H]:finish-all => rose_prune
+                WARM[-PT12H]:succeed-all => rose_prune
             """
 
 [runtime]

@wxtim
Copy link
Contributor Author

wxtim commented Jan 8, 2025

Does this PR want to be against the release branch or master?

@oliver-sanders
Copy link
Member

Either, only the changelog and version number differ.

@@ -128,7 +128,7 @@ while (($# > 0)); do
--help)
rose_help
exit
:;;
;;
Copy link
Member

Choose a reason for hiding this comment

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

Gotta do this for the other branches too.

Copy link
Member

Choose a reason for hiding this comment

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

(I had no idea : was an alias for true)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only the other branch with an exit in it.

@wxtim wxtim requested a review from oliver-sanders January 8, 2025 15:29
@MetRonnie MetRonnie changed the title excluded shellcheck error SC2317 Fix shellcheck error SC2317 & failing test Jan 8, 2025
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

I cannot reproduce the Mac OS test failures locally (12.7)

$ rtb t/rose-task-run/13-app-arch-cmd-out.t t/rose-task-run/30-app-arch-opt-source.t t/rose-app-run/05-file.t t/rose-task-env/02-360day-cycling.t 
t/rose-task-run/13-app-arch-cmd-out.t ..... ok                          
t/rose-task-run/30-app-arch-opt-source.t .. ok                          
t/rose-app-run/05-file.t .................. ok                          
t/rose-task-env/02-360day-cycling.t ....... ok   
All tests successful.

Possibly just a CI setup issue.

@oliver-sanders oliver-sanders merged commit 25fea8b into metomi:master Jan 8, 2025
5 of 6 checks passed
@wxtim wxtim deleted the exclude-SC2317 branch January 8, 2025 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants