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

manage_vhost sometimes breaks and redis version update results a fatal error during deploy #93

Conversation

stijnbernards
Copy link
Contributor

This MR solves 2 things:

  1. manage_vhost is ran before the current symlink has been updated. This results in errors during nginx setup and breaks automatic deployments. It also seems that the error which this may result fully stops the automatic nginx reload
  2. hypernode-systemctl settings redis_version {$config->getVersion()} --block" asks for a yes/no input. Which is not provided by deploy. If redis is upgraded to 7.x during deploy the deployment will never finish since the prompt will always come up even if redis is already on version 7.x

Copy link
Collaborator

@tdgroot tdgroot left a comment

Choose a reason for hiding this comment

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

Hey @stijnbernards! Thanks for your patches, much appreciated!

@@ -37,13 +37,14 @@ public function configureWithTaskConfig(TaskConfigurationInterface $config): ?Ta

task('deploy:nginx', [
Copy link
Collaborator

Choose a reason for hiding this comment

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

So now it first places nginx files and after that it runs HMV. Sounds OK to me, but what if the vhost doesn't exist on the node yet? Have you tested that scenario as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not yet, but I believe the deploy creates the www.shop.nl folders in the nginx directory aswell, not sure if creating a new vhost will then remove this. Let me see if I can test this somewhere :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you verify this or drop this part of the patch? Then we can merge!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try and verify this change soon but it's the most important fix of this MR. I'll create a new MR to fix the hypernode settings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just verified this change and it does infact fix my issue when creating an entirely new vhost during deploys when it does not exist yet. Same goed for the deploy:varnish:activate step as this step uses the current folder

@tdgroot
Copy link
Collaborator

tdgroot commented Mar 5, 2024

You have some merge conflicts, can you update your branch? Then we can get this merged for release :)

@stijnbernards
Copy link
Contributor Author

You have some merge conflicts, can you update your branch? Then we can get this merged for release :)

Done :)

@tdgroot tdgroot merged commit 9b0730c into ByteInternet:master Mar 6, 2024
5 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants