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

potential fix for CI #83

Closed
wants to merge 4 commits into from
Closed

potential fix for CI #83

wants to merge 4 commits into from

Conversation

kostrykin
Copy link
Member

@kostrykin kostrykin commented Nov 9, 2023

The requirement is that the CI deploys tools even if planemo lint raises an error during merge.

The current configuration of the CI sets--fail_level passed to planemo shed_lint to error:
https://github.com/BMCV/galaxy-image-analysis/actions/runs/6811812716/job/18523060276#step:7:355
To my understanding, this makes the linter fail only if it produces an error (but not if it produces warnings).
https://help.galaxyproject.org/t/make-a-repository-installable/8156/6

What we want instead, is, that the linter never fails during merge, not even if an error is encountered! However, only the options warn and error are allowed:

$ planemo shed_lint --help |grep fail_level
  --fail_level [warn|error]

Thus, the required behavior is out of scope of fail_level. It can only be achieved by skipping linting during merge. This should be implemented by this PR (hopefully, never digged myself into GitHub actions before).

xref #82 #71


FOR CONTRIBUTOR:

  • - I have read the CONTRIBUTING.md document and this tool is appropriate for the galaxy-image-analysis 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)
  • - Tools added/updated by this PR (if any) comply with the Naming and Annotation Conventions for Tools in the Image Community in Galaxy (or please explain why they do not)

@kostrykin kostrykin closed this Nov 9, 2023
@kostrykin kostrykin reopened this Nov 9, 2023
@kostrykin kostrykin requested a review from bgruening November 9, 2023 13:44
@kostrykin kostrykin mentioned this pull request Nov 9, 2023
6 tasks
@bernt-matthias
Copy link
Contributor

This is not needed IMO. For deployment lint runs with --fail-level error anyways.

Having --fail-level warn during PRs seems a nice feature since it usually leads to higher quality tools.

@kostrykin
Copy link
Member Author

kostrykin commented Nov 9, 2023

Having --fail-level warn during PRs seems a nice feature since it usually leads to higher quality tools.

Thanks, you are absolutely right, I changed this by accident. I fixed that in 69065ea.

@bernt-matthias
Copy link
Contributor

Sorry, this is not what I meant .. warn is even stricter that fail, i.e. if it fails with error it will fail with warn as well.

Give me a moment.

Ultimately biii needs to be added here .. which will effect only with the next galaxy release.

@kostrykin
Copy link
Member Author

kostrykin commented Nov 9, 2023

Sorry, this is not what I meant .. warn is even stricter that fail, i.e. if it fails with error it will fail with warn as well.

Yes, this is why I have changed it from error to warn for pull requests (because it's stricter). For merging, planemo lint would be skipped completely due to f976b91:

     - name: Planemo lint
       if: ${{ github.event_name == 'pull_request' }}

Ultimately biii needs to be added here .. which will effect only with the next galaxy release.

Yes, this is what we had already done in 00a3d03, and why we wanted to bypass linting for merging for the moment, until the next Galaxy release is there.

@bernt-matthias
Copy link
Contributor

#84

@kostrykin kostrykin closed this Nov 10, 2023
@kostrykin kostrykin deleted the ci_update branch November 14, 2023 12: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.

2 participants