Skip to content

Zizmor fixes#756

Draft
weiji14 wants to merge 5 commits intodevelopmentfrom
zizmor-fixes
Draft

Zizmor fixes#756
weiji14 wants to merge 5 commits intodevelopmentfrom
zizmor-fixes

Conversation

@weiji14
Copy link
Copy Markdown
Member

@weiji14 weiji14 commented Apr 22, 2026

Follow up to #754 to apply more security related fixes.

Will need to check https://github.com/icesat2py/icepyx/security/code-scanning?query=is%3Aopen+branch%3Adevelopment+tool%3Azizmor (after this PR is merged into the development branch) to ensure all issues have been resolved

TODO:

@weiji14 weiji14 self-assigned this Apr 22, 2026
@weiji14 weiji14 added the afternoon_contribution This issue should be resolvable in an afternoon or so of work. label Apr 22, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 22, 2026

Binder 👈 Launch a binder notebook on this branch for commit 07914d0

I will automatically update this comment whenever this PR is modified

Binder 👈 Launch a binder notebook on this branch for commit 305ece3

Binder 👈 Launch a binder notebook on this branch for commit 42192b9

Binder 👈 Launch a binder notebook on this branch for commit c44c0f2

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 22, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.22%. Comparing base (31e208a) to head (c44c0f2).

Additional details and impacted files
@@             Coverage Diff              @@
##           development     #756   +/-   ##
============================================
  Coverage        77.22%   77.22%           
============================================
  Files               42       42           
  Lines             3231     3231           
  Branches           401      401           
============================================
  Hits              2495     2495           
  Misses             603      603           
  Partials           133      133           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member Author

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Ok, have fixed most issues except for one that is more complicated. See below.

@@ -2,10 +2,15 @@ name: AddBinderBadge
on:
pull_request_target:
Copy link
Copy Markdown
Member Author

@weiji14 weiji14 Apr 22, 2026

Choose a reason for hiding this comment

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

Secure way would be to use pull_request instead of pull_request_target trigger. E.g. following https://mybinder.readthedocs.io/en/latest/howto/gh-actions-badges.html#example-2-comment-with-a-binder-badge-in-response-to-a-comment

Yes, slightly more complicated, but I've done it before at pangeo-data/pangeo-docker-images#631. This will require a change in behaviour, in that someone (with the proper permissions) has to write a comment with /binder to have the Binder button show up. I can go ahead with this if that's ok?

Comment on lines 11 to 12
workflow_run:
workflows: [Update UML diagrams]
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This workflow_run trigger is apparently dangerous, but also I'm not too sure how to avoid it 🙃. I think the key is that we want the unit tests to be re-ran after the UML diagram commit, need to think of how it can be done more safely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

afternoon_contribution This issue should be resolvable in an afternoon or so of work.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant