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

Create pulumi container #1711

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).
  • You have marked this pull request as a draft and added '[WIP]' to the title if needed (if you're not yet ready to merge).
  • You have formatted your code using appropriate automated tools (for example ./tests/AutoFormat_Powershell.ps1 -TargetPath <path to file or directory> for Powershell).

⤴️ Summary

Ensure that dsh context create also creates the pulumi storage container

🌂 Related issues

Closes #1710

🔬 Tests

Tested on fresh deployment

@jemrobinson jemrobinson requested a review from JimMadge January 24, 2024 09:01
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.

LGTM 👍.

I had to look around the code a bit because I couldn't recall exactly what I had done.

I remember thinking it is a bit odd to import things from the config classes in "context" space because you should create the context before a config.

But these are hardcoded values, so if they belong most naturally in the config I think that is fine. At least, not a bug or priority to move.

@jemrobinson
Copy link
Member Author

Agreed that this should probably live in the Context class but I noticed you already had it as part of ConfigSectionPulumi. I'll leave it up to you whether to fix this now or merge this PR as-is.

@JimMadge JimMadge merged commit 9e695cb into alan-turing-institute:python-migration Jan 24, 2024
9 checks passed
@JimMadge
Copy link
Member

JimMadge commented Jan 24, 2024

I'm not really sure. From a "structure of the program" perspective, it would neatly fit in the Context. I think I might have convinced myself it didn't conceptually fit there as the context doesn't really know about Pulumi. However, in the case of the name of the storage account that is part of the context, it totally does.

Added to an existing Issue #1689.

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.

2 participants