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

Fixes 6608: unusable venv after template copy, python not executable #6623

Merged
merged 4 commits into from
Jun 4, 2021

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Mar 25, 2021

Toward fixing #6608.

To reproduce 6608:

#!/bin/sh

mkdir template
pushd template
pulumi new --yes aws-python --name "template-6608-1"
popd

mkdir user-project
pushd user-project
pulumi new --yes ../template --name "user-project-6608-1"
popd

What happens is that the second pulumi new copies venv but forgets to mark files like python executable. It also ignores any symlink structure, if any... Symlinks become regular files.

Option 1: as coded in the PR, propagate more permission bits from source to dest, so executable files remain executable after template expansion, but everything is at least 0600.

Option 2: drop 0600, use exact permissions from source file. This did NOT fix the bug when using os.Stat - because of symlink handling.. begs the question of what should template expansion do with symlinks anyway? Copying symlinks as symlinks its not possible when the contents are being modified by the logic in here.

Option 3: skip virtual environment from the template copying process. Where could we do this cleanly? If we can do it cleanly it might be the best as then we will let Python manage virtual environment folder including deciding which one is a symlink and which one is not; we do not mess with that. It seems that upon copying the template, Pulumi up does re-initialization of the virtual env (if configured to manage it in the yaml). So that'd be ideal perhaps.

@t0yv0 t0yv0 linked an issue Mar 25, 2021 that may be closed by this pull request
@justinvp
Copy link
Member

Thanks, @t0yv0! I want to take a look over this before merging. Hopefully I’ll be able to get to it tomorrow, but if not, Monday.

@t0yv0
Copy link
Member Author

t0yv0 commented Mar 26, 2021

Oh right - the user has found a workaround so there is no rush on this one.

@t0yv0 t0yv0 marked this pull request as ready for review April 20, 2021 20:02
@justinvp
Copy link
Member

Our stance on this has generally been that venv should not be committed in the template repo. If your template has specific dependencies, include them in requirements.txt and Pulumi will automatically create the virtual environment and install the requirements.txt dependencies into it.

Option 1: as coded in the PR, propagate more permission bits from source to dest, so executable files remain executable after template expansion, but everything is at least 0600.

That said, this still seems like a reasonable change to make

@t0yv0
Copy link
Member Author

t0yv0 commented Apr 20, 2021

Can still repro on 3.x. Accepting the fix I think is better than leaving as-is, although there's better way to fix it.

@t0yv0
Copy link
Member Author

t0yv0 commented Apr 20, 2021

Our stance on this has generally been that venv should not be committed in the template repo. If your template has specific dependencies, include them in requirements.txt and Pulumi will automatically create the virtual environment and install the requirements.txt dependencies into it.

Hm that sounds good but should we then teach Pulumi CLI to exclude venv from consideration when it scans template folders and venv is accidentally present? I think this happens when an existing project is used as a template before the user commits it cleanly to a template repo.

Copy link
Member

@lblackstone lblackstone left a comment

Choose a reason for hiding this comment

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

This seems like a reasonable improvement to me.

Copy link
Member

@justinvp justinvp left a comment

Choose a reason for hiding this comment

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

Sorry, for some reason I thought I'd already approved this. LGTM.

Hm that sounds good but should we then teach Pulumi CLI to exclude venv from consideration when it scans template folders and venv is accidentally present?

Possibly. We've attempted to keep the template code language agnostic, but perhaps we could do some language-specific things if we can come up with a proper factoring, like calling the language host to do some processing?

@t0yv0 t0yv0 merged commit 3e1dc52 into master Jun 4, 2021
@pulumi-bot pulumi-bot deleted the anton/fix-6608 branch June 4, 2021 14:03
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.

Permissions error when running `pulumi new <path_to_repo_folder>
3 participants