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

Created a release builder workflow #2165

Merged
merged 56 commits into from
Dec 17, 2022

Conversation

fballiano
Copy link
Contributor

Since in the future I hope OpenMage moves some of its code (coming from external projects, like Cm_RedisSession, Cm_Cache_Backend_Redis, Pelago_Emogrifier, Zend Framework) outside of its repo (for delegation of responsibility and easier upgrades) we need a release builder that pulls this external code using composer and places it in "old style" directories (like lib/Cm or app/code/community/Cm/RedisSession).

The release builder workflow that I'm publishing with this PR does exactly that, upon tagging a new release it downloads every composer dependancy, re-creates the oldstyle directories, cleansup, zips and updates the release tag with the newly created file.

You can see the result of a test release on my fork of OM here: https://github.com/fballiano/magento-lts/releases/tag/vprova123.1.6

This PR is strictly dependant on #2138 because they both won't work if they're not both merged.

@justinbeaty
Copy link
Contributor

I like this! I'm wondering why though we need to do this:

mkdir -p app/code/community/Cm/RedisSession; cp -R vendor/colinmollenhour/magento-redis-session/app/code/local/Cm/RedisSession/* app/code/community/Cm/RedisSession/

When magento-hackathon/magento-composer-installer should be able to install this module from its modman file (although it would have gone into the app/code/local folder.)

Either way, this is really nice!

@fballiano
Copy link
Contributor Author

This is to maintain compatibility for all the people that are not using composer nor modmad, and generating a zipfile that's exactly how it always was since the beginning of time to now :-)

@justinbeaty
Copy link
Contributor

This is to maintain compatibility for all the people that are not using composer nor modmad, and generating a zipfile that's exactly how it always was since the beginning of time to now :-)

I do understand that, what I mean is that the composer install --ignore-platform-reqs should have been able to install the colinmollenhour/magento-redis-session module to app/code/local without the need to manually copy those files from vendor. We would still require a mv app/code/local/Cm app/code/community step to keep it consistent with the current structure, though.

@fballiano
Copy link
Contributor Author

sorry 😅

I think it only does it if the "magento-root-dir" is set correctly in the composer.json (although it didn't do it on my local tests) but anyway I preferred to keep the module in the app/code/community directory (as it is now in the repo)

@justinbeaty
Copy link
Contributor

sorry 😅

I think it only does it if the "magento-root-dir" is set correctly in the composer.json (although it didn't do it on my local tests) but anyway I preferred to keep the module in the app/code/community directory (as it is now in the repo)

Ah, that could be it indeed. I'm certainly not opposed to how you have it, I just thought it might be easier to have the magento-hackathon/magento-composer-installer repo do it. Especially if there were ever other Mage modules required this way, but I'm not sure there will ever be.

I downloaded the build zip file, I'll take a look through it all.

@Flyingmana
Copy link
Member

This PR is strictly dependant on #2138 because they both won't work if they're not both merged.

why is it necessary? Not standing in a way, but I would feel more comfortable when we first have a release where we have the workflow, and only later move dependencies out when we verified the downloads are working properly.

@fballiano
Copy link
Contributor Author

This PR is strictly dependant on #2138 because they both won't work if they're not both merged.

why is it necessary? Not standing in a way, but I would feel more comfortable when we first have a release where we have the workflow, and only later move dependencies out when we verified the downloads are working properly.

It's only necessary since the workflow is built to move the libraries downloaded via composer. If those libraries are managed "the old way" there's actually no need for this specific workflow (the way I've done it).

@sreichel
Copy link
Contributor

DISCUSS: imho .... i'd not care about the "old way". Nowadays composer should be available everywhere. Not?

However ... nice work, but ...

  1. why --ignore-platform-reqs?
  2. why move CM from app/local to app/community scope?
  3. have you tested cp command? Default deploy strategy for composer is symlink instead of copy an should be enforced in composer.json.

Maybe something like this ...

    "extra": {
        "magento-deploystrategy": "copy",
        "magento-deploystrategy-dev": "symlink",
        "magento-root-dir": "htdocs",
        "magento-force": true,
        "magento-core-package-type": "magento-source",
        "auto-append-gitignore": true
    },

@fballiano
Copy link
Contributor Author

DISCUSS: imho .... i'd not care about the "old way". Nowadays composer should be available everywhere. Not?

I see your point and I kinda agree but as far as I understood somebody from our team prefers to maintain as much backward compatibility as possible

  1. why --ignore-platform-reqs?

Probably because of some php extensions (it's been some months and I don't remember anymore) but at the end of the day it's not necessary that the build environment is actually a "run" environment, I'll check back again when possible.

  1. why move CM from app/local to app/community scope?

This was mainly to have exactly the same directory structure we have now on our repo, where we have app/code/community/Cm and, for people that extract the release tgz on an existing docroot, this way the redis module doesn't get duplicated.
I mean, it's not super necessary... I liked it because the built release was exactly the same of what we are releasing now.

  1. have you tested cp command? Default deploy strategy for composer is symlink instead of copy an should be enforced in composer.json.

I'll try but point 2 is still, I'm not that much opinionated on that, although I prefer when modules install themselves in the "community" scope

@fballiano
Copy link
Contributor Author

So, I've changed the action to run on php8 instead of the default 7.3, the ignore-platform-reqs is still required (it's used in other actions that we already have in out repo anyway) because otherwise "composer install" will not run without installing gd, sodium and soap, which are not required for building.

Problem is, if we build using php8 it will probably use some symfony packages that are not compatible with php7 so I'd like to know what @Flyingmana thinks about that.

Either we don't generate any realease (and people will have to run composer install), or should we generate as many releases as php versions?

@Flyingmana
Copy link
Member

we build a release package, which should replace the current .tgz download.
It should be as runnable without needing to execute composer.
It should support the "upload via FTP GUI client", because thats something our users do use. (and which can become very complicated when we would deploy using symlinks)

the " ignore-platform-reqs" mostly because the used php version or available extensions doesnt matter for during the build. Why do we need to check for PDO for building a .tgz File?

In most other github actions it was also, because we wanted to run it in environments, which are not yet officially supported. Or not anymore supported. It makes things easier, and I got not aware of any possible problems.

Using a more recent PHP version should be ok. Then its good enough to support the workflow. And if they need it for specific other PHP versions, then they still can pay someone to contribute a build for this one.
But dont bother making the php version depending on the build environment, its easier to run a composer config platform.php 8.0 right before and has the same effect as long as no plugins are breaking with the currently used php version.

@sreichel
Copy link
Contributor

sreichel commented Aug 6, 2022

the " ignore-platform-reqs" mostly because the used php version or available extensions doesnt matter for during the build. Why do we need to check for PDO for building a .tgz File?

I agree with ignoring PDO (--ignore-platform-req=ext-* should work), but not with ignoring php version (not sure if setting/faking platform in composer is suiteable).

If we use composer dependencies, that rely on differt php version, we should/can not ignore php version requirements.

@fballiano
Copy link
Contributor Author

after last modifications, here's the newly generated release:
https://github.com/fballiano/openmage/releases/tag/vprova123.3.9

@sreichel
Copy link
Contributor

sreichel commented Dec 6, 2022

Last commit (removing --no-dev) was not so good.

I think dev-tool are only for composer users.

@fballiano
Copy link
Contributor Author

Last commit (removing --no-dev) was not so good.

I think dev-tool are only for composer users.

I didn’t understand, do you think we should include dev files? Cause it’s making the zip 10mb bigger. I think they should be left out.

@sreichel
Copy link
Contributor

sreichel commented Dec 6, 2022

I should go to sleep ... thought you removed --no-dev flag ...

Nice work 👍

sreichel
sreichel previously approved these changes Dec 6, 2022
@sreichel
Copy link
Contributor

sreichel commented Dec 9, 2022

@fballiano Should we remove Cm_Redis until symlink error is fixed?

@fballiano
Copy link
Contributor Author

@fballiano Should we remove Cm_Redis until symlink error is fixed?

for me I wouldn't care since (if I understood it correctly) it's a problem only when switching the composer install from dev to no-dev right? so for release preparation and workflow should never be a problem right?

@sreichel
Copy link
Contributor

We have to remove CM-Credis.

Please check lib/Credis after composer install (delete vendor before). It is empty or not created. Tested both ... copy and symlink.

@fballiano fballiano mentioned this pull request Dec 14, 2022
4 tasks
@fballiano
Copy link
Contributor Author

We have to remove CM-Credis.

Please check lib/Credis after composer install (delete vendor before). It is empty or not created. Tested both ... copy and symlink.

@sreichel answered in #2799 (comment) but I think everything works as it is (I'm testing this branch with redis cache now and it's working fine).

@fballiano fballiano merged commit 10efe8b into OpenMage:1.9.4.x Dec 17, 2022
@fballiano fballiano deleted the first_release_builder_workflow branch December 17, 2022 09:01
@fballiano
Copy link
Contributor Author

yes!

@github-actions
Copy link

Unit Test Results

1 files  1 suites   0s ⏱️
0 tests 0 ✔️ 0 💤 0 ❌
7 runs  5 ✔️ 2 💤 0 ❌

Results for commit 10efe8b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
composer Relates to composer.json environment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants