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

Add an option to build magento artifact from the repository #3456

Merged
merged 1 commit into from
Jan 30, 2023
Merged

Add an option to build magento artifact from the repository #3456

merged 1 commit into from
Jan 30, 2023

Conversation

guvra
Copy link
Contributor

@guvra guvra commented Jan 23, 2023

  • Bug fix #…?
  • New feature?
  • BC breaks?
  • Tests added?
  • Docs added?

Currently, the Magento 2 recipe only allows to build the artifact file from the current working directory.

This works perfectly fine when building the artifact on a platform dedicated to deployments (e.g. gitlab CI), but it makes it nearly impossible to deploy from a local environment for the following reasons:

  • A dev environment runs in developer mode.
  • The vendor directory contains dev packages
  • There is a possibility that the current working directory contains uncommitted files.

The goal of this PR is to add an option that works around this limitation, by cloning the project repository in a temporary folder, and creating the artifact file from this repository.

Example usage:

dep artifact:build localhost --option build_from_repo=1
<?php

namespace Deployer;

require 'recipe/magento2.php';

localhost()
    ->set('local', true);

host('prod')
    ->setHostname('myprod')
    ->setRemoteUser('www')
    ->setDeployPath('/var/www/html');

set('repository', 'git@xxxxxxxxxxxxxxxx.git');

If no repository was specified (repository option), an error is displayed.

Advantages:

  • No BC changes, it's just a new option.
  • Very few changes to the current code of the recipe.
  • Allows to easily deploy Magento projects from a local environment (more user friendly than having to create a build environment or setting up automatic deployment on a github/gitlab CI).

@guvra
Copy link
Contributor Author

guvra commented Jan 26, 2023

docgen workflow was failing, I amended the commit to update the documentation.

@Schrank
Copy link
Contributor

Schrank commented Jan 26, 2023

@schmengler @peterjaap I'm not totally sure I understand what happens here. Can I have a second opinion? From my point of view, I'm happy to merge it.

@peterjaap
Copy link
Contributor

@Schrank I have 4 MR's to look at by the other Fabian :P @schmengler I'll try to get around to it tomorrow!

Copy link
Contributor

@schmengler schmengler left a comment

Choose a reason for hiding this comment

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

The feature looks fine, but documentation is missing

@guvra
Copy link
Contributor Author

guvra commented Jan 26, 2023

I added better documentation about the feature:

  • A comment explain how to use build_from_repo
  • A comment explains that repository must be overridden if build_from_repo is set to true.

@schmengler
Copy link
Contributor

@guvra question: How would one specify which branch should be built?
I imagine something like dep artifact:build localhost --option build_from_repo=1 --option build_from_branch=release/1.0 would be useful

@guvra
Copy link
Contributor Author

guvra commented Jan 26, 2023

I don't know if I should add a comment about recommending to set the target option (by default it uses HEAD, in most cases you need to deploy a tag or a specific branch instead).

@schmengler
Copy link
Contributor

I don't know if I should add a comment about recommending to set the target option (by default it uses HEAD, in most cases you need to deploy a tag or a specific branch instead).

posted at the same time :D yes, please do! As you see I was confused

@guvra
Copy link
Contributor Author

guvra commented Jan 26, 2023

@schmengler I just thought of another way of doing it:

  • If repository is set, then the artifact is built from a clean copy of the repository with the specified target (HEAD by default, but can be overridden with --branch, --tag or --revision).
  • Otherwise, it's built from the current working directory (current behavior)

With this logic, we don't need build_from_repo anymore.

It's more simple, but maybe also more confusing/dangerous? Can be considered as a compatibility break too.

@schmengler
Copy link
Contributor

tbh, I wasn't aware of these CLI arguments, but that seems to be exactly the way they are intended to be used, like in the update_code recipe:

$repository = get('repository');
$target = get('target');

Since the magento recipe currently does not use the repository or target variables I don't see a real backwards compatibility issue

@peterjaap
Copy link
Contributor

peterjaap commented Jan 30, 2023

LGTM! @antonmedv merge at will

@antonmedv antonmedv merged commit 30c66ce into deployphp:master Jan 30, 2023
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.

None yet

5 participants