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

Split esc/ and esc/cmd into separate modules. #205

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

pgavlin
Copy link
Member

@pgavlin pgavlin commented Dec 19, 2023

These changes peel esc/cmd out of the esc module s.t. the latter has no dependency on pulumi/pulumi.

Unfortunately, the esc CLI itself does require a pulumi/pulumi dependency so that it can share credential management with the pulumi CLI.

These changes should at least simplify the dependency graph for pulumi/pulumi somewhat so that pulumi/pulumi/sdk can depend on esc without circularity.

Unfortunately, there is still circularity between esc/cmd and pulumi/pulumi/pkg. We could resolve this by splitting off yet another module that contains the CLI implementation but not the default implementations of various bits that refer to pulumi/pulumi/pkg. That would probably give us the following modules:

  • github.com/pulumi/esc <-- no pulumi dependencies
  • github.com/pulumi/esc/cmd <-- pkg/ and sdk/ dependencies
  • github.com/pulumi/esc/cmd/cli <-- sdk/ dependency

pulumi/pulumi/sdk could then freely depend on pulumi/esc and pulumi/pulumi/pkg could depend on github.com/pulumi/esc/cmd/cli, all without circularity.

The cost, of course, is a more complicated release process for esc, wherein the commit/tag flow becomes:

  1. tag esc/v1.2.3
  2. update esc/cmd/cli to consume esc/v1.2.3, then commit + tag esc/cmd/cli/v1.2.3
  3. update esc/cmd to consume esc/cmd/cli/v1.2.3, then commit + tag esc/cmd/v1.2.3

That said, that process is probably worth the simplicity of reasoning about dependencies.

Thoughts?

@justinvp
Copy link
Member

Thanks for digging into this, Pat!

pulumi/pulumi/sdk could then freely depend on pulumi/esc and pulumi/pulumi/pkg could depend on github.com/pulumi/esc/cmd/cli, all without circularity.

This would be ideal. If others find it unpalatable, I hope we can at least consider the changes in this PR so that pulumi/pulumi/sdk can depend on esc without circularity.

That said, that process is probably worth the simplicity of reasoning about dependencies.

It's unfortunate that it complicates the release process, but it's likely worth it for the simpler dependencies. We can automate the release process to make it easier on ourselves.

@pgavlin
Copy link
Member Author

pgavlin commented Dec 19, 2023

Thanks for digging into this, Pat!

pulumi/pulumi/sdk could then freely depend on pulumi/esc and pulumi/pulumi/pkg could depend on github.com/pulumi/esc/cmd/cli, all without circularity.

This would be ideal. If others find it unpalatable, I hope we can at least consider the changes in this PR so that pulumi/pulumi/sdk can depend on esc without circularity.

SG. I'll roll those changes into this PR.

That said, that process is probably worth the simplicity of reasoning about dependencies.

It's unfortunate that it complicates the release process, but it's likely worth it for the simpler dependencies. We can automate the release process to make it easier on ourselves.

Yeah I agree with that.

@komalali
Copy link
Member

Agreed with what y'all have discussed. Unfortunate about the release process but fixing the circular dependency seems worth it.

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