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

MDTT-38: Fix for CI failure. #24

Merged
merged 11 commits into from
Nov 16, 2023
Merged

MDTT-38: Fix for CI failure. #24

merged 11 commits into from
Nov 16, 2023

Conversation

sadeesh-sdet
Copy link
Contributor

Description

This pull request addresses the following changes:

  1. PHPStan Improvement: Used a combination of type hinting and explicit checks to assist PHPStan in understanding the types involved and resolving static analysis errors in the issetField function.

  2. Dependency Security Check: Added a security check for installed dependencies using the symfonycorp/security-checker-action@v4 action.

The rationale behind these changes is to enhance code quality by addressing PHPStan static analysis issues and to ensure the security of the project by checking for known vulnerabilities in dependencies.

Ticket: MDTT-38

Type of Change

  • fix (A bugfix)

Scope (component, file, endpoint, etc.):

  • PHPStan improvement: issetField function in [file path]
  • Dependency security check: CI/CD workflow configuration

@codecov
Copy link

codecov bot commented Sep 16, 2023

Codecov Report

Attention: 33 lines in your changes are missing coverage. Please review.

Comparison is base (65885e9) 20.47% compared to head (078521b) 18.40%.
Report is 1 commits behind head on main.

❗ Current head 078521b differs from pull request most recent head 5a5b5ad. Consider uploading reports for the commit 5a5b5ad to get more accurate results

Files Patch % Lines
src/Test/DefaultTest.php 0.00% 24 Missing ⚠️
src/RunCommand.php 0.00% 9 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main      #24      +/-   ##
============================================
- Coverage     20.47%   18.40%   -2.07%     
- Complexity      163      168       +5     
============================================
  Files            20       20              
  Lines           503      603     +100     
============================================
+ Hits            103      111       +8     
- Misses          400      492      +92     

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

@sadeesh-sdet
Copy link
Contributor Author

@hussainweb cc @shwetaneelsharma As this project was originated as a part of coe, kindly review this PR when you have some time.

Copy link
Member

@hussainweb hussainweb left a comment

Choose a reason for hiding this comment

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

CodeCov is throwing a lot of errors. If the code coverage is not important, consider disabling the check temporarily.

src/Test/DefaultTest.php Show resolved Hide resolved
src/Test/DefaultTest.php Outdated Show resolved Hide resolved
@sadeesh-sdet
Copy link
Contributor Author

sadeesh-sdet commented Sep 25, 2023

CodeCov is throwing a lot of errors. If the code coverage is not important, consider disabling the check temporarily.

@hussainweb Thank you for your feedback and for reviewing the pull request.

I'd like to add a note that I didn't intentionally add the CodeCov integration or the code coverage check; it was preexisting. I'm uncertain about its importance, especially since I've noticed that PRs have been merged despite code coverage issues.

Considering your suggestion, I agree that we should temporarily disable the code coverage check. This will prevent the check failures and allow us to merge the PR promptly. We can then discuss the necessity of code coverage and decide whether to re-enable the check in the future.

@hussainweb
Copy link
Member

@sadeesh-sdet, if the changes here are meeting your needs for the moment, let's merge this and then work on all the comments in separate PRs. What do you say?

@sadeesh-sdet
Copy link
Contributor Author

@sadeesh-sdet, if the changes here are meeting your needs for the moment, let's merge this and then work on all the comments in separate PRs. What do you say?

@hussainweb I think we've already addressed everything except code coverage enablement and its suitable exploration, which we can work on in the future based on priority!

Copy link
Member

@hussainweb hussainweb left a comment

Choose a reason for hiding this comment

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

Sorry, it took so long to review. Please see my comments and I think these should be simple changes.

src/Exception/InvalidArgumentException.php Outdated Show resolved Hide resolved
src/Test/DefaultTest.php Outdated Show resolved Hide resolved
@sadeesh-sdet
Copy link
Contributor Author

Sorry, it took so long to review. Please see my comments and I think these should be simple changes.

@hussainweb Likewise, I was tied up with urgent client tasks. I'll attend to the feedback shortly.

@sadeesh-sdet
Copy link
Contributor Author

@hussainweb I have addressed the PR feedbacks. Please review and merge the changes.

@hussainweb hussainweb merged commit 0b960e0 into axelerant:main Nov 16, 2023
1 check passed
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