-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Feature/magento2 artifact deployment #3317
Feature/magento2 artifact deployment #3317
Conversation
chgrp for all files, chmod g+rwxs for directories, chmod g+rw for files
Hi there, After a discussion in the magento_de slack channel I was wondering, if it makes sense to directly include
in the recipe? Question probably for @peterjaap |
Good work. I've found the jalogut one is now incompatible with PHP 8.1, so I might try this PR out instead of creating a PR over there. |
Let’s wait review from the @peterjaap |
I was discussing this MR lately in the magento_de slack group, and there will be some improvements coming from that side. |
recipe/magento2.php
Outdated
}); | ||
|
||
desc('Clears the opcache, cache tool required.'); | ||
task('cache:clear:opcache', function() { |
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.
Exists an option to reset opcache with Deployer:
https://deployer.org/docs/7.x/contrib/cachetool
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.
Yeah, @sprankhub told me that already, I should put that out.
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.
which we needed to improve though: #3363
Killing consumers shouldn't be necessary. Magento should start consumers when they're needed and have them exit when there's nothing in the queue. I guess the exception is when you would set https://experienceleague.adobe.com/docs/commerce-operations/configuration-guide/files/config-reference-envphp.html#consumers_wait_for_messages to |
recipe/magento2.php
Outdated
run('rm -rf {{release_path}}/{{artifact_file}}'); | ||
}); | ||
|
||
desc('Provides env.php for build.'); |
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 do you create an env.php file for the build? There is no database etc, everything we need should be in config.php. At least this should be optional
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.
Can second this
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.
Ok, the idea behind this is/was the way static assets are generated.
Since on production and dev they differ, I thougth it would be a good Idea, to have them in an env.php
which broke my deployment. But probably you are right, If env.php
settings override conf.php
settings, then we should just omit this part, do they.
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.
env.php
overrides config.php
indeed, and should not be committed. config.php
(if configured correctly) is sufficient to run static content deploy and is committed to the repo.
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.
So, OK, I'll put this out soon then...
@peterjaap Well, I don't mind putting this out of the MR, anyhow I needed this during a deployment phase, where I was heavily working on consumers. They had like 50 tasks to complete and were "laying" there waiting for new messages, which they would complete with the old code rather then the new changed one, which caused big trouble. |
@peterjaap @wilfriedwolf the default setting for consumers is to wait until 1000 messages before restarting when cron runs. However, as far as I know, there is a poison pill for consumers to kill themselves when the static content deploy hash is changed. That said, I agree that I'm not sure whether killing consumer processes should be in the scope of this module. (Unless if it somehow interferes with the actual deployment process itself.) Also see https://developer.adobe.com/commerce/php/development/components/message-queues/object-states/ edit: okay so it turns out the current poison pill implementation isn't great as it requires the var dir to be shared across al releases (which still probably is fine for people that use this module): |
Regarding the consumers problem and the poison pill: Anyhow it was a question whether to put the code in the MR or not, actually it is not, so I just leave it out and keep it (and the fruitful discussion here) in mind. |
So to conclude:
Anything I oversaw? |
Actually, after reading magento/magento2#23540 (comment) I think I've changed my mind, maybe leave the consumers part in... some good points are discussed there, and it seems to also be included in the capistrano extension. Does anyone else have any thoughts? |
recipe/magento2.php
Outdated
throw new GracefulShutdownException("You can only deploy to a non localhost"); | ||
} else { | ||
add('shared_files', get('additional_shared_files') ?? []); | ||
add('shared_dirs', get('additional_shared_dirs') ?? []); |
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.
would you add default values for these in the configuration section and document them there? This is a commonly needed feature and should not stay undocumented
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.
Not quite sure if I get it. 'shared_files'
and 'shared_dirs'
are defined in the standard magento2 recipe. So I think I should not comment these. and the 'additional_shared_files'
and 'additional_shared_dirs'
are just provided to add some nonstandard files or dirs coming from your shop.
So anyhow I can comment this, but how would I do it, since the documentation is created 🤔
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.
The documentation is created from comments before set()
calls. I'll do it in our fork and show you
Other changes I already implemented: https://github.com/integer-net/deployer/pulls?q=is%3Apr+
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.
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.
BTW: I had to move the add('...
-part in a task (depoy:additional-shared
), just like in your PR it did not add the yml settings, I guess because of timing.
desc('Adds additional files and dirs to the list of shared files and dirs');
task('deploy:additional-shared', function () {
add('shared_files', get('additional_shared_files'));
add('shared_dirs', get('additional_shared_dirs'));
});
recipe/magento2.php
Outdated
invoke('artifact:prepare'); | ||
|
||
invoke('magento:upgrade:db'); | ||
invoke('magento:config:import'); |
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.
We have a problem with all those invoke()
, apparently they only execute this one task and not anything added via before()
or after()
, which makes the recipe hard to extend for custom needs.
Maybe task()->select()
can be used to only run it on non localhost? If not, is this restriction so important?
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.
Me too (just noticed, because I had to add magepack:build :)
Could the problem possibly be solved by defining a task
desc('Forces the tasks to be executed locally and sets the pathes');
task('build:force-local', function () {
if(currentHost()->get('local')) {
set('deploy_path', '.');
set('release_path', '.');
set('current_path', '.');
} else {
throw new GracefulShutdownException("Artifact can only be built locally, you provided a non local host");
}
});
desc('Builds an artifact.');
task('artifact:build', [
'build:force-local',
'build:remove-generated',
'deploy:vendors',
'magento:compile',
'magento:deploy:assets',
'artifact:package'
]);
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.
Would also apply to the deployment which must not be local!
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.
yes, here's my working solution: integer-net#10 (didn't bother to force non-local, but the same would work there too)
recipe/magento2.php
Outdated
invoke('magento:deploy:assets'); | ||
invoke('artifact:package'); | ||
} else { | ||
throw new GracefulShutdownException("Artifact can only be built locally, you provided a non local host"); |
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've asked Anton for clarification here about local tasks: #2679 (comment)
but if there's no cleaner way I would turn this into an array of tasks without invoke()
too (see below)
…artifact-deployment
- uses contrib/cachetool - removes task build:prepare-env - comments additional_shared_files and additonal_shared_dirs - avoids invoke()
So, I removed and added as discussed. |
@antonmedv @peterjaap Anything else here to do? I'm using this version (my own composer package) for a month now and I do not see any open questions problems (except maybe a final statement from @peterjaap)? |
I'm ok to merge. @peterjaap ? |
Thank you @antonmedv 🧡 Brace yourself for more PRs |
@wilfriedwolf Do you have any example of deploy.php file that implements the artifact tasks? I'm trying to use it but I don't know how to build the artifact (the task edit: "solved" it with the following code: localhost()
->set('local', true); |
@guvra well, actually I use
which basically is the same as your "workaround". The sense behind that restriction is/was to avoid accidently building artifacts on remote machine. Don't know if this is necessary.. |
@antonmedv Thanks a lot for merging! |
Let’s add some documentation in recipe as comments. |
That would be neat 👍 I managed to make it work on a gitlab ci automated deployment, but I got a few issues with the I solved it by creating a dummy env.php file in the gitlab ci script:
|
Can we add this pseudo env as a step in recipe? |
@guvra @antonmedv Actually this is not necessary. I had that approach (providing a special For the assets you would need
|
@wilfriedwolf Thanks for the heads up, it works perfectly 👍 FYI, I added a section in my deploy.php file that allows to build the artifact from a development environment (e.g. local docker compose container): set('repository', 'git@xxxxxxxxx');
// Custom code for deployment from dev environment
set('build_dir', null);
after('build:prepare', function () {
$buildDir = (string) get('build_dir');
if ($buildDir=== '') {
return;
}
set('deploy_path', $buildDir);
set('current_path', $buildDir);
set('current_path', $buildDir);
run('rm -rf {{release_or_current_path}}');
run('git clone {{repository}} {{release_or_current_path}}');
run('git checkout --force {{target}}'); Then I just have to specify e.g. |
* origin/master: (33 commits) Update default cachetool version to 9.0.0 (PHP 8.1 compatible) (deployphp#3462) Update UPGRADE.md (deployphp#3458) update writable deploy recipe to use release_or_current_path (deployphp#3449) Fix PostgreSQL provisioning Fix PostgresDB user creation Show failed tasks for CI-environments (deployphp#3446) Fix NodeJS 12 warning: update EndBug/add-and-commit action (deployphp#3442) Replace unmaintained satackey/push-prebuilt-action by a maintained fork (deployphp#3437) Fix Node.js 12 actions are deprecated (deployphp#3435) Fix set-output command is deprecated and will be disabled soon (deployphp#3436) Add "new version" banners on release Revert "Fixes deployphp#3365 (deployphp#3400)" Make Deployer work with PHP 8.2 (deployphp#3422) Update symfony/console to ^5.4.9 Update symfony/console to ^5.4.9 Test on php 8.2 as well Revert "Composer update" Composer update [automatic] Update docs with bin/docgen Feature/magento2 artifact deployment (deployphp#3317) ...
how do we use artifact deployment exactly? does someone have a sample deploy.php file maybe? |
@wilfriedwolf will be cool to add some docs right in the recipe comments. |
Hej @antonmedv I am definitvely willing to do so, but I have no clue how. Looking on the generated Best regards, Willi |
@peterjaap @schmengler Do you have a hint? |
You can just add a comment at the top of the recipe. |
What I could figure out until now is that I basically need to run these 2 command's: (for the moment ignoring #3454) ./vendor/bin/dep artifact:build -vvv --option build_from_repo=true However it fails for me with this: It is a little strange because the artifact file is made like this: And when I run the untar: /usr/bin/tar -xzpf ... artifact.tar.gz -C .../releases/26 then the repo folder is untarred into artifacts/repo inside .../releases/26, instead of on top of .../releases/26, so that's why the magento bin is not found. Edit: all this in MacOs Ventura |
@akosglue You must build the artifact file from a local environment. First, you need to add this line in deploy.php: localhost()
->set('local', true); If you're using a yaml file instead of deploy.php, IIRC the syntax is the following: hosts:
localhost:
local: true To build the artifact, there are two modes:
Notes:
Then, to deploy:
Warning: config.php file must contain the list of the stores/websites, as well as the CSS/JSS compilation settings. This is because Magento won't have access to a database during the artifact generation, so the relevant data must be stored in config.php instead. Example of config.php file: return [
'modules' => [
// ...
],
'MAGE_MODE' => 'production',
'scopes' => [
'websites' => [
'admin' => [
'website_id' => '0',
'code' => 'admin',
'name' => 'Admin',
'sort_order' => '0',
'default_group_id' => '0',
'is_default' => '0'
],
'base' => [
'website_id' => 1,
'code' => 'base',
'name' => 'Main Website',
'sort_order' => '0',
'default_group_id' => '1',
'is_default' => '1'
]
],
'groups' => [
[
'group_id' => '0',
'website_id' => '0',
'code' => 'default',
'name' => 'Default',
'root_category_id' => '0',
'default_store_id' => '0'
],
[
'group_id' => '1',
'website_id' => '1',
'code' => 'main_website_store',
'name' => 'Main Website Store',
'root_category_id' => '2',
'default_store_id' => '1'
]
],
'stores' => [
'admin' => [
'store_id' => '0',
'code' => 'admin',
'website_id' => '0',
'group_id' => '0',
'name' => 'Admin',
'sort_order' => '0',
'is_active' => '1'
],
'default' => [
'store_id' => '1',
'code' => 'default',
'website_id' => '1',
'group_id' => '1',
'name' => 'Default Store View',
'sort_order' => '0',
'is_active' => '1'
]
]
],
'system' => [
'default' => [
'dev' => [
'js' => [
'enable_js_bundling' => '0',
'minify_files' => '1',
'merge_files' => '1'
],
'css' => [
'minify_files' => '1',
'merge_css_files' => '1'
],
'static' => [
'sign' => '1'
],
'template' => [
'minify_html' => '1'
]
]
]
]
]; |
Let’s add it as documentation in recipe. |
Thanks @guvra I also needed to add '--strip=2' to the artifact:extract command but I think this is a MacOS specific issue. |
@antonmedv Submitted a PR with the documentation after digging a little in the |
@akosglue Do you use |
I used tar (/usr/bin/tar) anyways, it work also with tar, I just needed to make this task override.
|
I guess you can use tar on mac, but then you maybe should add some |
Yes, it seems to be cleaner.
|
But I don't know if there is a check for MacOS, so better use gtar on Mac :) |
…xplain artifact deployment (#3510) * adds sticky to writable recipe chgrp for all files, chmod g+rwxs for directories, chmod g+rw for files * adds artifact deployment for magento2 recipe * Updates Documentation * removes mode that has been submitted in other MR * adds divers discussion results from PR #3317 - uses contrib/cachetool - removes task build:prepare-env - comments additional_shared_files and additonal_shared_dirs - avoids invoke() * updates docs * corrects additional shared * expands DocGen for artifact deployment and adds documentation to the magento2 recipe * corrects missing capital and typo --------- Co-authored-by: Anton Medvedev <anton@medv.io>
New feature?
BC breaks?
Tests added?
Docs added?