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

Split asset compilation #3326

Merged
merged 16 commits into from
Mar 24, 2023
Merged

Conversation

renttek
Copy link
Contributor

@renttek renttek commented Sep 30, 2022

This PR adds the improvements from #3311
The new variables have comments explaining the supported configuration options/structure

recipe/magento2.php Outdated Show resolved Hide resolved
recipe/magento2.php Outdated Show resolved Hide resolved
Copy link
Contributor

@peterjaap peterjaap left a comment

Choose a reason for hiding this comment

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

Process Simon's typo fixes

peterjaap and others added 2 commits October 4, 2022 16:55
Co-authored-by: Simon Sprankel <sprankhub@users.noreply.github.com>
Co-authored-by: Simon Sprankel <sprankhub@users.noreply.github.com>
@sprankhub
Copy link
Contributor

@renttek, mind checking the pipeline issue?

@renttek
Copy link
Contributor Author

renttek commented Oct 18, 2022

@sprankhub I'll them pipeline issues later this week. (Latest at the weekend 🙂)

@sprankhub
Copy link
Contributor

Really, @renttek? 😛

@renttek
Copy link
Contributor Author

renttek commented Oct 25, 2022

Sorry, it slipped my mind a bit. I fixed the issues and updated the PR 🙂

if ($useDefaultLanguages) {
$themes = implode('-t ', $themes);

run("{{bin/php}} {{release_or_current_path}}/bin/magento setup:static-content:deploy --area=$staticContentArea --content-version={{content_version}} {{static_deploy_options}} $defaultLanguages $themes -j {{static_content_jobs}}");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add --force here by default? Otherwise deploy mode has to be "production" and on a build server you usually don't have that configured

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 think this makes sense

Copy link
Contributor

Choose a reason for hiding this comment

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

The implode should be also reviewed.
If I have this:

array (
  0 => 'Hyva/default',
  1 => 'Magento/luma',
)

The implode makes this:

Hyva/default-t Magento/luma

However it should be this:

-t Hyva/default -t Magento/luma

Copy link
Contributor

Choose a reason for hiding this comment

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

the implode is updated and the force option is added as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

@akosglue great, could you also fix the merge conflicts?

Copy link
Contributor

Choose a reason for hiding this comment

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

seems to be resolved now

Copy link
Contributor

Choose a reason for hiding this comment

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

@peterjaap any chance to merge this?


// backend themes to deploy. Only used if split_static_deployment=true
// This setting supports the same options/structure as {{magento_themes}}
set('magento_themes_backend', ['Magento/backend']);
Copy link
Contributor

Choose a reason for hiding this comment

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

the default value should be ['Magento/backend' => null]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right. I'll fix this later

Copy link
Contributor

Choose a reason for hiding this comment

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

I have committed the change for this.

@antonmedv antonmedv closed this Jan 9, 2023
@sprankhub
Copy link
Contributor

Why has this been closed, @antonmedv?

@antonmedv antonmedv reopened this Jan 30, 2023
@antonmedv
Copy link
Member

Please resolve conflicts.

@@ -48,6 +48,7 @@
"react/http": "^1.5",
"symfony/console": "^5",
"symfony/polyfill-php80": "^1.22",
"symfony/polyfill-php81": "^1.26",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need that requirement here in this PR?

I think it is required anyways, for example:

symfony/dependency-injection v5.4.20 requires symfony/polyfill-php81 (^1.22)
and
web-token/jwt-framework v2.2.11 requires symfony/dependency-injection (^4.2|^5.0)
and
magento/product-community-edition 2.4.5-p1 requires web-token/jwt-framework (^v2.2.7)

So when you have a magento installation you will have polyfill-php81 (since magento 2.4.4), or I am wrong somewhere?

@akosglue
Copy link
Contributor

akosglue commented Mar 6, 2023

I have a couple of commits for this PR, if you give access @renttek I can push it?

Pushing to https://github.com/renttek/deployer
remote: Permission to renttek/deployer.git denied to akosglue.
fatal: unable to access 'https://github.com/renttek/deployer/': The requested URL returned error: 403

# Conflicts:
#	composer.lock
#	docs/recipe/magento2.md
#	recipe/magento2.php
@akosglue
Copy link
Contributor

akosglue commented Mar 6, 2023

Well, I tried but something is amiss. (the hash of the lock file I think) @renttek
I first updated the master branch of https://github.com/renttek/deployer from the original repo (so the fork is up-to-date in comparison with deployer)
Then I merged the master onto the feature branch: https://github.com/renttek/deployer/tree/split-asset-compilation
I got 3 conflicts: the recipe was easily resolved, the lock file was wrongly solved and the the docs changes were ignored because afterwards I regenerated.
So I think I have to take a look at the hash of the lock file.
Edit: all tests passed on the feature branch after the merge.

@antonmedv
Copy link
Member

So, what’s the status? Merging?

@akosglue
Copy link
Contributor

Yes, and hopefully it can fit into a new release, then we have a nice feature set in deployer for magento deployments.

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.

7 participants