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

Circos and PR workflow fixes #5242

Merged
merged 8 commits into from
Apr 12, 2023

Conversation

bernt-matthias
Copy link
Contributor

FOR CONTRIBUTOR:

  • - I have read the CONTRIBUTING.md document and this tool is appropriate for the tools-iuc repo.
  • - License permits unrestricted use (educational + commercial)
  • - This PR adds a new tool or tool collection
  • - This PR updates an existing tool or tool collection
  • - This PR does something else (explain below)

@bernt-matthias bernt-matthias changed the base branch from circos-fixes to main April 11, 2023 16:23
@bernt-matthias bernt-matthias mentioned this pull request Apr 11, 2023
@bernt-matthias
Copy link
Contributor Author

I think you also need to change line 319 above

Lets try :) Thanks for the suggestion.

Wondering if it might be a solution to forbid skipped here

if: ${{ needs.combine_outputs.result != 'success' && needs.combine_outputs.result != 'skipped' }}
(and maybe in all other tasks of this job except for the file size task).

Somehow I remember that we added this only to allow for PR that do not change tools .. but this might be better solved by #5219

@bernt-matthias
Copy link
Contributor Author

xref 4cc19b2

Copy link
Member

@hexylena hexylena left a comment

Choose a reason for hiding this comment

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

looks fine for me

@wm75
Copy link
Member

wm75 commented Apr 12, 2023

Somehow I remember that we added this only to allow for PR that do not change tools .. but this might be better solved by #5219

Not quite, I'm afraid. #5219 only serves as a shortcut for obvious cases of "no tool can possibly be affected by this". You can still have changes witihin tool directories that are not supposed to trigger tests. So #5219 can act as a prefilter that gets simple cases out of the way fast, but you still need the more fine-grained logic of the planemo action for everything that passes that first check.

@bernt-matthias
Copy link
Contributor Author

You can still have changes witihin tool directories that are not supposed to trigger tests.

Do you have an example?

@wm75
Copy link
Member

wm75 commented Apr 12, 2023

You can still have changes witihin tool directories that are not supposed to trigger tests.

Do you have an example?

Currently, the only example is changes to tools listed in .tt_skip.
Was thinking that tools without a shed.yml file would be another case, but those cause a failure, not a skipped test.

If we are talking about any skipped steps, not just planemo testing, something I'd like to implement soonish is skipping R linting when no R-related files have been touched.

python restrictions should come via conda, removing it seems to allow to
resolve the environment.

also gawk seems not to be used in the tools, so it should come via
conda.

Co-authored-by: Nicola Soranzo <[email protected]>
@bernt-matthias
Copy link
Contributor Author

And the last question before merging: should we bump? I would say no...

@bernt-matthias bernt-matthias enabled auto-merge (squash) April 12, 2023 14:25
@bernt-matthias bernt-matthias merged commit 0cfd88d into galaxyproject:main Apr 12, 2023
@bernt-matthias bernt-matthias deleted the circos-fixes branch April 12, 2023 15:58
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.

4 participants