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

[StepSecurity] Apply security best practices #1606

Conversation

step-security-bot
Copy link
Contributor

Summary

This pull request is created by StepSecurity at the request of @Zeitsperre. Please merge the Pull Request to incorporate the requested changes. Please tag @Zeitsperre on your message if you have any questions related to the PR.

Security Fixes

Pinned Dependencies

GitHub Action tags and Docker tags are mutatble. This poses a security risk. GitHub's Security Hardening guide recommends pinning actions to full length commit.

Keeping your actions up to date with Dependabot

With Dependabot version updates, when Dependabot identifies an outdated dependency, it raises a pull request to update the manifest to the latest version of the dependency. This is recommended by GitHub as well as The Open Source Security Foundation (OpenSSF).

Maintain Code Quality with Pre-Commit

Pre-commit is a framework for managing and maintaining multi-language pre-commit hooks. Hooks can be any scripts, code, or binaries that run at any stage of the git workflow. Pre-commit hooks are useful for enforcing code quality, code formatting, and detecting security vulnerabilities.

Feedback

For bug reports, feature requests, and general feedback; please email [email protected]. To create such PRs, please visit https://app.stepsecurity.io/securerepo.

Signed-off-by: StepSecurity Bot [email protected]

@Zeitsperre Zeitsperre self-requested a review January 18, 2024 16:56
Copy link

github-actions bot commented Jan 18, 2024

Welcome, new contributor!

It appears that this is your first Pull Request. To give credit where it's due, we ask that you add your information to the AUTHORS.rst and .zenodo.json:

  • The relevant author information has been added to AUTHORS.rst and .zenodo.json

Please make sure you've read our contributing guide. We look forward to reviewing your Pull Request shortly ✨

NOTE: This PR is autogenerated.

@github-actions github-actions bot added the CI Automation and Contiunous Integration label Jan 18, 2024
Copy link

github-actions bot commented Jan 18, 2024

Warning
This Pull Request is coming from a fork and must be manually tagged approved in order to perform additional testing.
Note
This Pull Request is approved!

@github-actions github-actions bot added sdba Issues concerning the sdba submodule. API Interfacing and User Concerns labels Jan 18, 2024
@Zeitsperre Zeitsperre force-pushed the stepsecurity_remediation_1705596976 branch from 9e1372d to 74f63e9 Compare January 18, 2024 20:35
@github-actions github-actions bot added the indicators Climate indices and indicators label Jan 19, 2024
@Zeitsperre Zeitsperre requested a review from aulemahal January 19, 2024 16:20
@Zeitsperre Zeitsperre self-assigned this Jan 19, 2024
Copy link
Collaborator

@Zeitsperre Zeitsperre left a comment

Choose a reason for hiding this comment

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

I think we're in business

@Zeitsperre Zeitsperre added the approved Approved for additional tests label Jan 19, 2024
@coveralls
Copy link

coveralls commented Jan 19, 2024

Coverage Status

coverage: 90.278% (-0.006%) from 90.284%
when pulling 65fc23c on step-security-bot:stepsecurity_remediation_1705596976
into 9ac718b on Ouranosinc:master.

@Zeitsperre
Copy link
Collaborator

@aulemahal I made a few small adjustments to the xclim code base. Can you quickly just verify that I haven't angered any guvectorized-based Eldritch Gods?

Copy link
Collaborator

@aulemahal aulemahal left a comment

Choose a reason for hiding this comment

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

I think this introduces a bit too many disabling comments. Each time a new dev will work on that code, the same (non-)issues would be raised by the pre-commit hooks, adding (IMO) too much noise and confusion.

I found this option here : https://pylint.readthedocs.io/en/latest/user_guide/configuration/all-options.html#signature-mutators Do you think adding map_groups and guvectorize to that list could solve the issues ?

xclim/indices/fire/_ffdi.py Outdated Show resolved Hide resolved
@Zeitsperre Zeitsperre requested a review from aulemahal January 19, 2024 20:34
@Zeitsperre
Copy link
Collaborator

Zeitsperre commented Jan 19, 2024

@aulemahal I couldn't get guvectorize to play nicely, but I've added some top-level pylint disable messages to deal with them. It's a known issue.

See: pylint-dev/pylint#3758

@@ -521,7 +522,7 @@ missing-member-max-choices = 1
mixin-class-rgx = ".*[Mm]ixin"

# List of decorators that change the signature of a decorated function.
# signature-mutators =
signature-mutators = ["xclim.sdba.base.map_groups"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

wouldn't adding "numba.guvectorize" solve the issues in xclim/indices/fire/_ffdi.py and xclim/sdba/nbutils.py ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

For the record, that doesn't work. Maybe one day this will be resolved.

Copy link
Collaborator

@aulemahal aulemahal left a comment

Choose a reason for hiding this comment

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

Fair enough then! Hopefully that gets fixed one day!

@Zeitsperre Zeitsperre merged commit 4aeabc2 into Ouranosinc:master Jan 22, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Interfacing and User Concerns approved Approved for additional tests CI Automation and Contiunous Integration indicators Climate indices and indicators sdba Issues concerning the sdba submodule.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants