-
Notifications
You must be signed in to change notification settings - Fork 396
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
Issue #813: Clear twig caches on deployments. #1151
Conversation
settings/blt.settings.php
Outdated
* custom code that changes the container, changing this identifier will also | ||
* allow the container to be invalidated as soon as code is deployed. | ||
*/ | ||
$deploy_id = file_get_contents(DRUPAL_ROOT . '/../deploy_id.txt'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use a file_exists() check here.
phing/tasks/deploy.xml
Outdated
<isset property="deploy.tag"/> | ||
<then> | ||
<!-- Write tag name for the benefit of environments like ACE that can't access it. --> | ||
<exec dir="${deploy.dir}" command="echo '${deploy.tag}' > deploy_id.txt" logoutput="true" checkreturn="true" level="${blt.exec_level}"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the commit hash so that this works for non-tags as well? E.g., git rev-parse HEAD
rather than echo '${deploy.tag}'
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest that we name this file deployment_identifier
rather than deploy_id.txt
for consistency.
@@ -139,6 +139,16 @@ | |||
</if> | |||
</target> | |||
|
|||
<target name="deploy:id" description="Writes deployment id information to artifact."> | |||
<if> | |||
<isset property="deploy.tag"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only do this for tags? I seems like we'd want it for all build artifacts.
I renamed the file to deployment_identifier and added a file_exists to catch notices. The only time you'd need to set this deployment identifier is for production deploys, since dev/stage don't have multiple web heads. And you'd only ever deploy tags to production. So this seemed like a simpler solution than commit hashes. I also thought commit hashes would be weird because the deployment_identifier would be the previous commit hash; it wouldn't match the current commit because writing the identifier obviously happens prior to the commit. I don't see why that would be bad, it's just weird. Let me know if you'd rather use the commit hash, that's fine with me. |
Makes sense. |
Fixes #813
Changes proposed:
deploy:id
step to deployments that writes current deploy tag (if present) to a file in the deploy dir.$settings['deployment_identifier']
accordingly.$settings['deployment_identifier']
to handle stale Twig caches on multiple web heads.I've tested this on a real project and confirmed that the identifier is set correctly, although I haven't been able to test that this actually fixes the problem with stale Twig templates, since that involves a production deploy. I'm fairly confident it will work though.