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

Migrate to python 3 + Invoke #61

Open
wants to merge 4 commits into
base: master
from
Open

Migrate to python 3 + Invoke #61

wants to merge 4 commits into from

Conversation

@lyrixx
Copy link
Member

lyrixx commented Mar 13, 2020

Fixes #45

@lyrixx lyrixx force-pushed the move-to-invoke branch 6 times, most recently from e2fa123 to ec5950e Mar 13, 2020
Pipfile Show resolved Hide resolved
@lyrixx lyrixx force-pushed the move-to-invoke branch 5 times, most recently from c81a582 to 316d9f6 Mar 13, 2020
invoke.py Outdated Show resolved Hide resolved
@lyrixx lyrixx force-pushed the move-to-invoke branch 2 times, most recently from 7779e91 to 43e73ee Mar 13, 2020
@lyrixx

This comment was marked as resolved.

Copy link
Member Author

lyrixx commented Mar 13, 2020

Here we go! We can now use python 3.
I remove fabric since it does not support python 3 and I replaced it by invoke.
Note: invoke is used by fabric 2

I need to manually test more things, but this PR is ready for review.
I also need to write a migration guide

@damienalexandre

This comment was marked as resolved.

Copy link
Member

damienalexandre commented Mar 13, 2020

You need to update the README.md file too as thre is "fabfile" references in the recipes.

@lyrixx lyrixx force-pushed the move-to-invoke branch 4 times, most recently from 6efad79 to 7375105 Mar 13, 2020
@lyrixx

This comment has been minimized.

Copy link
Member Author

lyrixx commented Mar 13, 2020

Hi !

This PR is finished 💚

I added an upgrade guide. I hope you will like it

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
UPGRADE-3.0.md Outdated Show resolved Hide resolved
UPGRADE-3.0.md Outdated Show resolved Hide resolved
UPGRADE-3.0.md Outdated Show resolved Hide resolved
UPGRADE-3.0.md Outdated Show resolved Hide resolved
UPGRADE-3.0.md Outdated Show resolved Hide resolved
UPGRADE-3.0.md Outdated Show resolved Hide resolved
UPGRADE-3.0.md Outdated Show resolved Hide resolved
README.dist.md Outdated Show resolved Hide resolved
README.dist.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@lyrixx lyrixx requested review from pyrech and damienalexandre Mar 18, 2020
.circleci/config.yml Show resolved Hide resolved
invoke.py Show resolved Hide resolved
invoke.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
@lyrixx lyrixx force-pushed the move-to-invoke branch from cb2975d to 8adf83a Mar 21, 2020
@lyrixx

This comment has been minimized.

Copy link
Member Author

lyrixx commented Mar 21, 2020

This PR is ready 💚
Go for merge?

@pyrech
pyrech approved these changes Mar 23, 2020
UPGRADE-3.0.md Outdated Show resolved Hide resolved
@lyrixx lyrixx force-pushed the move-to-invoke branch from 8adf83a to ab1c3b6 Mar 23, 2020
@lyrixx lyrixx force-pushed the move-to-invoke branch from ab1c3b6 to 1e83f1b Mar 23, 2020
@lyrixx

This comment has been minimized.

Copy link
Member Author

lyrixx commented Mar 23, 2020

I have rebased against master + squashed some commits + fixed the worker integration

@lyrixx lyrixx force-pushed the move-to-invoke branch from 1e83f1b to 40356fe Mar 23, 2020
Copy link
Member

damienalexandre left a comment

I had it working on Windows BUT only with a modification of the path:

-        ' '.join('-f \'' + c.root_dir + '/infrastructure/docker/' + file + '\'' for file in c.docker_compose_files),
+        ' '.join('-f infrastructure/docker/' + file + '' for file in c.docker_compose_files),

I did commit some minor Windows specific correction but this one is not. WDYT?


[requires]
python_version = "2.7"
python_version = "3.7"

This comment has been minimized.

Copy link
@damienalexandre

damienalexandre Mar 23, 2020

Member

I got this warning:

Warning: Your Pipfile requires python_version 3.7, but you are using 3.8.2 (C:\Users\mobma.\d\S\python.exe).

And I don't see why we don't allow more minor versions here.

This comment has been minimized.

Copy link
@lyrixx

lyrixx Mar 24, 2020

Author Member

Me neither. We can relax this requirements. Or even drop this line entirely

UPGRADE-3.0.md Show resolved Hide resolved
UPGRADE-3.0.md Show resolved Hide resolved
cmd = 'docker-compose -p %s %s %s' % (
c.project_name,
' '.join('-f \'' + c.root_dir + '/infrastructure/docker/' + file + '\'' for file in c.docker_compose_files),

This comment has been minimized.

Copy link
@damienalexandre

damienalexandre Mar 23, 2020

Member

Something doesn't work well here, I get this error:

.OSError: [Errno 22] Invalid argument: ".\'C:\Users\mobma\Dev\docker-starter/infrastructure/docker/docker-compose.builder.yml'"

The root_dir is quite full. But if I replace it with ./ I still get an escaped ':

.FileNotFoundError: [Errno 2] No such file or directory: ".\'./infrastructure/docker/docker-compose.builder.yml'"

Using directly this works:

' '.join('-f infrastructure/docker/' + file + '' for file in c.docker_compose_files),

This comment has been minimized.

Copy link
@lyrixx

lyrixx Mar 24, 2020

Author Member

Could you try to run the command with -e ?

inv -e up

This comment has been minimized.

Copy link
@damienalexandre

damienalexandre Mar 27, 2020

Member

Here is the result:

docker-compose -p app -f 'C:\Users\mobma\Dev\docker-starter/infrastructure/docker/docker-compose.builder.yml' -f 'C:\Users\mobma\Dev\docker-starter/infrastructure/docker/docker-compose.yml' -f 'C:\Users\mobma\Dev\docker-starter/infrastructure/docker/docker-compose.worker.yml' build --build-arg PROJECT_NAME=app --build-arg USER_ID=1000 php-base

Doesn't look so bad.

This comment has been minimized.

Copy link
@damienalexandre

damienalexandre Mar 27, 2020

Member

What is strange is that I can run this command directly from Powershell and it works.

@damienalexandre damienalexandre force-pushed the move-to-invoke branch from 1a9e8f8 to 73d3a52 Mar 23, 2020
@lyrixx

This comment has been minimized.

Copy link
Member Author

lyrixx commented Mar 24, 2020

@damienalexandre Thanks a lot for the review and test and fix 👍

I did commit some minor Windows specific correction but this one is not. WDYT?

We must keep the full qualified path. We should find a way to make it works

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.