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

Symplify autowiring and remove backticks #19

Merged
merged 26 commits into from
May 30, 2024

Conversation

parijke
Copy link
Contributor

@parijke parijke commented May 21, 2024

  • adhere to the most recent best practices
  • remove ORM dependency (using DBAL)
  • simplify autowiring and service definitions
  • remove backticks from select statements
  • using 6.1 bundle definition
  • moved services and routes from src/Resources/config to ./config

fixes #15

Advantages: really simple services.yml. If you want to override the autowiring defaults, you could define an entry in the services.yaml.

How it works:
All services which implements HealthCheckInterface are tagged with openconext.monitor.health_check, that is why is is not needed in the Extension anymore. The TaggableIterator is initialized with al services tagged like this. That is why the CompilerPass becomes obsolete.

Just as flexible as before, just much simpeler. Thanks Symfony!
Also see: https://symfony.com/doc/current/bundles/extension.html

@parijke parijke requested a review from MKodde May 21, 2024 16:51
Copy link

codecov bot commented May 21, 2024

Codecov Report

Attention: Patch coverage is 21.42857% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 45.08%. Comparing base (62f51d5) to head (642f803).
Report is 9 commits behind head on main.

Current head 642f803 differs from pull request most recent head e83bde5

Please upload reports for the commit e83bde5 to get more accurate results.

Files Patch % Lines
src/HealthCheck/DoctrineConnectionHealthCheck.php 0.00% 6 Missing ⚠️
src/Controller/InfoController.php 0.00% 2 Missing ⚠️
src/OpenConextMonitorBundle.php 0.00% 2 Missing ⚠️
src/Controller/HealthController.php 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main      #19      +/-   ##
============================================
+ Coverage     41.17%   45.08%   +3.91%     
- Complexity       52       65      +13     
============================================
  Files            12       13       +1     
  Lines           153      173      +20     
============================================
+ Hits             63       78      +15     
- Misses           90       95       +5     

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

Copy link
Member

@MKodde MKodde left a comment

Choose a reason for hiding this comment

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

A very nice effort to actually lever some useful newer Symfony constructions into the project. And a good call to stop using Doctrine ORM but opt for DBAL instead 🎩

I have some minor concerns listed below. Please have at them!

README.md Outdated Show resolved Hide resolved
src/Controller/InfoController.php Show resolved Hide resolved
src/HealthCheck/DoctrineConnectionHealthCheck.php Outdated Show resolved Hide resolved
src/HealthCheck/HealthCheckInterface.php Outdated Show resolved Hide resolved
src/OpenConextMonitorBundle.php Show resolved Hide resolved
@MKodde
Copy link
Member

MKodde commented May 22, 2024

@tacman does this solve your issues as proposed in #15? The other suggestion to allow for setting a custom query is still in our pipeline. We recently ran into an issue where checking existence of a certain table was very useful.

I am unsure if it was an error as the test results do not change after this fix. This is a smell of an untrusted test
@parijke parijke force-pushed the feaure/autowiring-simplification-and-backticks branch from e57b0da to a350061 Compare May 22, 2024 09:20
Copy link
Member

@MKodde MKodde left a comment

Choose a reason for hiding this comment

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

One todo remains. I think adding the service tag is important to help existing users to upgrade with NOOP.

Copy link
Contributor Author

@parijke parijke left a comment

Choose a reason for hiding this comment

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

Reverted back to using tags

@parijke parijke merged commit 8a2d0b0 into main May 30, 2024
3 checks passed
@parijke parijke deleted the feaure/autowiring-simplification-and-backticks branch May 30, 2024 11:10
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.

backticks don't work in postgres for database health check
2 participants