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.

Sign up for GitHub

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

Jump to bottom

Add check files component #133

Merged
merged 3 commits into from
Jan 15, 2025
Merged

Add check files component #133

merged 3 commits into from
Jan 15, 2025

Conversation

Harry-Ramsey
Copy link
Contributor

@Harry-Ramsey Harry-Ramsey commented Dec 27, 2024 •
edited
Loading

Description

Enable check_files.py for TF-PSA-Crypto. Closes #51.

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

  • changelog not required because: testing enhancement.
  • framework PR provided Mbed-TLS/mbedtls-framework: #109
  • mbedtls PR not required.
  • tests provided.

Notes for the submitter

Please refer to the contributing guidelines, especially the
checklist for PR contributors.

Help make review efficient:

  • Multiple simple commits
    • please structure your PR into a series of small commits, each of which does one thing
  • Avoid force-push
    • please do not force-push to update your PR - just add new commit(s)
  • See our Guidelines for Contributors for more details about the review process.

@Harry-Ramsey Harry-Ramsey force-pushed the check-files-development branch from 550a1c4 to 5f6ed4e Compare December 27, 2024 11:24
@Harry-Ramsey Harry-Ramsey self-assigned this Jan 7, 2025
@Harry-Ramsey Harry-Ramsey force-pushed the check-files-development branch 3 times, most recently from e409293 to 4e5c203 Compare January 9, 2025 10:55
@Harry-Ramsey Harry-Ramsey added enhancement New feature or request needs-review Every commit must be reviewed by at least two team members needs-reviewer This PR needs someone to pick it up for review size-xs Estimated task size: extra small (a few hours at most) labels Jan 9, 2025
@Harry-Ramsey Harry-Ramsey mentioned this pull request Jan 9, 2025
Merged
3 tasks
@Harry-Ramsey Harry-Ramsey changed the title Check files development Add check files component Jan 9, 2025
@ronald-cron-arm ronald-cron-arm self-requested a review January 9, 2025 14:13
ronald-cron-arm
ronald-cron-arm previously approved these changes Jan 10, 2025
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@valeriosetti valeriosetti self-requested a review January 10, 2025 11:57
@ronald-cron-arm ronald-cron-arm removed the needs-reviewer This PR needs someone to pick it up for review label Jan 13, 2025
@valeriosetti
Copy link
Contributor

It seems to me that the framework reference is not correct. It's pointing to 2e1d022f9038358e0472b0deaed65a0ed3374912 but it should instead point to Mbed-TLS/mbedtls-framework@2988e24. Isn't it?

I know that this PR is going to be updated anyway once the framework PR is merged, but for the sake of CI testing I think it would be better to test the proper version.

@Harry-Ramsey
Copy link
Contributor Author

Yes, good spot. I shall update the framework pointer now. CI should continue passing with any luck :)

@Harry-Ramsey Harry-Ramsey force-pushed the check-files-development branch from 4e5c203 to 4465e98 Compare January 13, 2025 08:56
valeriosetti
valeriosetti requested changes Jan 13, 2025
View reviewed changes
tests/scripts/components-basic-checks.sh
#### Basic checks
################################################################

component_tf_psa_crypto_check_files () {
Copy link
Contributor

Choose a reason for hiding this comment

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

The originating issue says we need 2 new components: component_tf_psa_crypto_check_files (added here) and component_tf_psa_crypto_check_python_files which I cannot find anywhere in the repo. Just to be sure, was it intentional to skip the 2nd?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, it was edited since I last saw it. Let me add a new component for check_python_files.

valeriosetti
valeriosetti reviewed Jan 13, 2025
View reviewed changes
tests/scripts/components-basic-checks.sh Outdated

components_tf_psa_crypto_check_python_files () {
msg "Lint: Python scripts"
./framework/scripts/check-python-files.sh
Copy link
Contributor

@valeriosetti valeriosetti Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry to bother you again, but why ./framework/ here and $FRAMEWORK on line 14?
IMO it would be better to keep the same pattern along the file and the one on line 14 looks better IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry forgot to edit that line. Both are the same but ideally for maintainability I should have used $FRAMEWORK.

@Harry-Ramsey Harry-Ramsey force-pushed the check-files-development branch from 9dcf1d6 to 6a945cc Compare January 13, 2025 13:30
valeriosetti
valeriosetti previously approved these changes Jan 13, 2025
View reviewed changes
Copy link
Contributor

@valeriosetti valeriosetti left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comment. LGTM :)

ronald-cron-arm
ronald-cron-arm previously approved these changes Jan 15, 2025
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@Harry-Ramsey Harry-Ramsey force-pushed the check-files-development branch from 6a945cc to 4dbc3b7 Compare January 15, 2025 14:22
@Harry-Ramsey
Add check_files component
5966e5d 
This commit enables a check_files component for TF-PSA-Crypto.

Signed-off-by: Harry Ramsey <[email protected]>
@Harry-Ramsey
Update framework for check_files.py
2c01684 
This commit updates the framework to enable check_files.py to run for
TF-PSA-Crypto.

Signed-off-by: Harry Ramsey <[email protected]>
@Harry-Ramsey
Add check python files component
a320a4b 
This commit adds the check python files component to TF-PSA-Crypto.

Signed-off-by: Harry Ramsey <[email protected]>
@Harry-Ramsey Harry-Ramsey force-pushed the check-files-development branch from 4dbc3b7 to a320a4b Compare January 15, 2025 14:38
ronald-cron-arm
ronald-cron-arm approved these changes Jan 15, 2025
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@ronald-cron-arm ronald-cron-arm added this pull request to the merge queue Jan 15, 2025
Merged via the queue into Mbed-TLS:development with commit 4ec115f Jan 15, 2025
3 checks passed
@ronald-cron-arm ronald-cron-arm mentioned this pull request Jan 16, 2025
Closed
3 tasks
@Harry-Ramsey Harry-Ramsey deleted the check-files-development branch January 21, 2025 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@valeriosetti valeriosetti valeriosetti approved these changes

@ronald-cron-arm ronald-cron-arm ronald-cron-arm approved these changes

Assignees

@Harry-Ramsey Harry-Ramsey

Labels
enhancement New feature or request needs-review Every commit must be reviewed by at least two team members size-xs Estimated task size: extra small (a few hours at most)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add file checks
3 participants
@Harry-Ramsey @valeriosetti @ronald-cron-arm