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

Fix Pulumi/dsh Python mismatch #2240

Merged
merged 1 commit into from
Oct 18, 2024
Merged

Conversation

jemrobinson
Copy link
Member

✅ Checklist

  • You have given your pull request a meaningful title (e.g. Enable foobar integration rather than 515 foobar).
  • You are targeting the appropriate branch. If you're not certain which one this is, it should be develop.
  • Your branch is up-to-date with the target branch (it probably was when you started, but it may have changed since then).

🚦 Depends on

n/a

⤴️ Summary

Set PULUMI_PYTHON_CMD to use the same Python executable that is being used by dsh.

Thanks for the pointer @craddm !

🌂 Related issues

Closes #2239

🔬 Tests

Tested locally, but we should test on the rc1 branch once this is merged into it

@jemrobinson jemrobinson requested a review from a team as a code owner October 17, 2024 15:39
Copy link

github-actions bot commented Oct 17, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  data_safe_haven/external/interface
  pulumi_account.py
Project Total  

This report was generated by python-coverage-comment-action

Copy link
Member

@JimMadge JimMadge left a comment

Choose a reason for hiding this comment

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

I'm not sure this will fix #2239 on its own. I don't think it is just the Python interpreter but also the PYTHONPATH.

@craddm
Copy link
Contributor

craddm commented Oct 18, 2024

Seems to work on my Docker container

@jemrobinson
Copy link
Member Author

Yeah, it also worked for me. I'm not sure why, but I assume it sets the path correctly behind the scenes?

@JimMadge
Copy link
Member

Interesting, maybe the interpreter "knows" where it's modules are, or the default path is relative to the executable file or something. If this works on its own though I don't see much benefit in also finding and setting the venv path 👍.

@jemrobinson
Copy link
Member Author

I tried the venv method and couldn't get it to work, so if this fix works for @helendduncan then I'm happy with it :)

@JimMadge
Copy link
Member

Seems to work on my Docker container

Reading this a second time, does that mean this works in an environment where it didn't work before (a pipx install), or it still works in your container where it worked previously?

@craddm
Copy link
Contributor

craddm commented Oct 18, 2024

Seems to work on my Docker container

Reading this a second time, does that mean this works in an environment where it didn't work before (a pipx install), or it still works in your container where it worked previously?

pipx didn't work previously - same issue with not finding the modules. Note this is standalone Docker container, not the .devcontainer that I usually use.

@JimMadge
Copy link
Member

Great, that sounds like a positive result for this change working then 🎉.

@jemrobinson
Copy link
Member Author

Confirmed working by @helendduncan

@jemrobinson jemrobinson merged commit da03fa7 into release-v5.0.1rc1 Oct 18, 2024
10 of 11 checks passed
@jemrobinson jemrobinson deleted the 2239-set-pulumi-venv branch October 18, 2024 11:29
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.

3 participants