Skip to content

Conversation

@algomaster99
Copy link
Contributor

Continuation of #1029

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Mar 29, 2025
@algomaster99
Copy link
Contributor Author

Checked for /home/macaron here. Except for https://github.com/oracle/macaron/actions/runs/13936281341/job/39005499709#step:8:1825, I refactored every logging statement. Maybe that statement has been removed in future versions.

@algomaster99 algomaster99 marked this pull request as ready for review March 29, 2025 16:39
@algomaster99
Copy link
Contributor Author

Once the CI checks are enabled, I can verify if I have taken care of all logging statments.

@algomaster99
Copy link
Contributor Author

The typing annotations check does not fail for me locally. And this seems to be part of the code that I did not change so is there some issue with the CI? I have an older version of Python the build 3.11.0 vs 3.11.11 in build.

Check typing annotations.................................................Failed
- hook id: mypy
- exit code: 1

src/macaron/database/views.py:147:59: error: Argument "callable_" to "execute_if" of "ExecutableDDLElement" has incompatible type "Callable[[BaseDDLElement, SchemaItem, Connection | None, list[Table] | None, Any | None, KwArg(Any)], bool]"; expected "DDLIfCallable | None"  [arg-type]
src/macaron/database/views.py:151:90: error: Argument "callable_" to "execute_if" of "ExecutableDDLElement" has incompatible type "Callable[[BaseDDLElement, SchemaItem, Connection | None, list[Table] | None, Any | None, KwArg(Any)], bool]"; expected "DDLIfCallable | None"  [arg-type]

@behnazh-w
Copy link
Member

The typing annotations check does not fail for me locally. And this seems to be part of the code that I did not change so is there some issue with the CI?

Yes. This PR fixes this typing error. Please rebase your branch on staging once the PR is merged.

@algomaster99
Copy link
Contributor Author

algomaster99 commented Mar 31, 2025 via email

@algomaster99
Copy link
Contributor Author

@behnazh-w I rebased and force-pushed. My unit tests were failing because of some coverage requirement so I was not able to push, but for now I disable the pre-push hook. I don't think so it should affect this PR.

See error log:


FAIL Required test coverage of 60.0% not reached. Total coverage: 32.57%
=========================== short test summary info ============================
ERROR src/macaron/slsa_analyzer/analyzer.py
ERROR src/macaron/slsa_analyzer/checks/detect_malicious_metadata_check.py
ERROR tests/slsa_analyzer/checks/test_detect_malicious_metadata_check.py
ERROR tests/slsa_analyzer/checks/test_detect_malicious_metadata_check.py
ERROR tests/slsa_analyzer/test_analyzer.py
ERROR tests/slsa_analyzer/test_analyzer.py
ERROR tests/test_main.py
ERROR tests/test_main.py
!!!!!!!!!!!!!!!!!!! Interrupted: 8 errors during collection !!!!!!!!!!!!!!!!!!!!
============================== 8 errors in 4.53s ===============================

@behnazh-w
Copy link
Member

@behnazh-w I rebased and force-pushed. My unit tests were failing because of some coverage requirement so I was not able to push, but for now I disable the pre-push hook. I don't think so it should affect this PR.

That's interesting. The unit tests seem to be passing on the CI, so there should be an issue with your local setup.

@tromai
Copy link
Contributor

tromai commented Apr 1, 2025

Hi @algomaster99 It seems like there are more error messages for this issues. Could you please also include here any other error messages from pytest (if available) ?

@algomaster99
Copy link
Contributor Author

algomaster99 commented Apr 1, 2025

so there should be an issue with your local setup.
Could you please also include here any other error messages from pytest (if available)

It was complaining about my outdated TOML file. I simply ran make upgrade and it is now fixed.

It seems like there are more error messages for this issues.

Hopefully, I fixed it my new commit 1ce3676 (#1032). (the message will be squashed anyway, right?)

@behnazh-w
Copy link
Member

Hopefully, I fixed it my new commit 1ce3676 (#1032). (the message will be squashed anyway, right?)

Thanks! Yes, we will squash the commits.

@algomaster99
Copy link
Contributor Author

Ready to merge 🎉
image

Thanks for help @tromai @behnazh-w !

@behnazh-w
Copy link
Member

behnazh-w commented Apr 3, 2025

@algomaster99 Thanks for the changes and we are almost ready to merge the PR. The commits however miss the Signed-off-by: Your Name <[email protected]> as instructed here, which can be done by the following git commit arg:

git commit --signoff

Please also rebase on staging again to get the latest changes.

@behnazh-w behnazh-w self-requested a review April 3, 2025 01:34
@algomaster99
Copy link
Contributor Author

@behnazh-w signed-off my commits. I overlooked it because I already sign my commits but it seems there is a difference between "signing" and "signing off".

I rebased too.

@behnazh-w behnazh-w merged commit f335ec0 into oracle:staging Apr 3, 2025
9 checks passed
@algomaster99 algomaster99 deleted the relative-path branch April 4, 2025 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

OCA Verified All contributors have signed the Oracle Contributor Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants