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

fix: creating a directory for a service container #1059

Closed
wants to merge 1 commit into from

Conversation

vbyndych
Copy link
Contributor

@vbyndych vbyndych commented Aug 10, 2023

Goal

Make sure cache directory exists before creating a service container file. This way we can ensure no error happen during later steps.

Changelog

  • fix: creating a directory as a service container requires it.

How to test / use

  1. Install application
  2. Remove data directory
  3. Run the command php index.php oat\generis\scripts\tools\ContainerCacheWarmup

@github-actions
Copy link

Version

Target Version 15.27.1
Last version 15.27.0

There are 0 BREAKING CHANGE, 0 feature, 1 fix

@@ -101,6 +101,10 @@ public function forceBuild(): ContainerInterface
return $this->legacyContainer;
}

if (!file_exists($this->cachePath)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we are trying to solve an infra issue in the application.

An env without the cache dir created is the problem, so the infra deployment should make sure it is created with right permissions before deploy anything

⚠️ Delegating it to app can have other side effects, example, app user might not have permissions to create the directory or maybe parent directories to not exist, etc

So we should solve this on infra level as we do not have the same issue with exnterprise customers, so probably there the pipeline is correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we sill want to create permissions here, we need to also validate if we have write permissions to create it and treat exception properly in case of no permissions, please :)

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

As we discussed in out call we can use oat\oatbox\cache\SetupFileCache to create the directory in a previous step before calling container warmup command.

Copy link
Contributor Author

@vbyndych vbyndych Aug 10, 2023

Choose a reason for hiding this comment

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

Yeah, thank you for the discussion. It seems like the problem with the pipeline has broader implications than I initially thought. That's why I think to create one more script similar to install or update which will take care of those things. Please check it out oat-sa/tao-core#3870. I decided not to put it inside the cache warmer itself because it is not his responsibility and in case we would change the pipeline later, we would still be able to use the application cache warmer without any modifications.

I'm closing this PR though as it is not needed anymore.

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