Refactor: move plugin kind to apitype #15946
Conversation
Changelog[uncommitted] (2024-04-25)Miscellaneous
|
glena
force-pushed
the
german/refactor-pluginkind
branch
from
c8e399f
to
5351677
Compare
sdk/go/common/workspace/plugins.go
Outdated
| const ( | ||
| // AnalyzerPlugin is a plugin that can be used as a resource analyzer. | ||
| AnalyzerPlugin PluginKind = "analyzer" | ||
| // LanguagePlugin is a plugin that can be used as a language host. | ||
| LanguagePlugin PluginKind = "language" | ||
| // ResourcePlugin is a plugin that can be used as a resource provider for custom CRUD operations. | ||
| ResourcePlugin PluginKind = "resource" | ||
| // ConverterPlugin is a plugin that can be used to convert from other ecosystems to Pulumi. | ||
| ConverterPlugin PluginKind = "converter" | ||
| // ToolPlugin is an arbitrary plugin that can be run as a tool. | ||
| ToolPlugin PluginKind = "tool" | ||
| ) |
There was a problem hiding this comment.
Do we need to re-export these as well? Otherwise I think this is still a backwards incompatible change.
Of course that makes it quite awkward to keep in sync,
There was a problem hiding this comment.
yes I just pushed it. It sucks tho... wonder how can we do it differently
The source of the circular dependency is this line https://github.com/pulumi/pulumi/pull/15945/files#diff-c59945d33c5fccb80d8b16ea994dfd17192bdd306b70f7feb07c73031fd88f72R855
because apitype depends on workspace in this line
pulumi/sdk/go/common/apitype/core.go
Line 356 in f6b405c
I could move all the deployment settings definitions under workspace and avoid all this refactor (which I dont like but might simplify everything). What do you think?
There was a problem hiding this comment.
Yeah, I agree that it kinda sucks. I guess on second thought we don't actually need to keep them in sync though, if we're introducing new plugin types we can just add it to the apitype package, as that won't be a breaking change, and people can move over to that.
If move the deployment settings definitions under workspace that also gets a bit awkward.
Another potential option would be to create a new package in pulumi/sdk/go/common/apitype/deployment and only have the deployment definitions there? Though I guess that doesn't work either because DeploymentSettings would still depend on apitype, so we'd just get the same dependency cycle again. And to break that we'd have to move all of that stuff to apitype/deployment, which means we have to re-export a bunch more stuff, which is not better.
So I guess this is maybe the best we can do unless someone can come up with a smarter idea.
There was a problem hiding this comment.
@tgummerer so I believe this is ready now. Not sure if the failed Downstream Codegen Tests status are something to look into.
Question, should we add a deprecation notice to the re-exported types redirecting people to use apitype?
There was a problem hiding this comment.
The codegen tests look like they failed to even create the downstream PR, and timed out. I'm trying to re-run them, but I think we can merge this either way, as it doesn't look like it's actually changing anything in codegen, other than some refactoring, so there's a pretty low chance of something breaking downstream.
I see you did add a deprecation notice already, and I think that's the right place to put it, I don't think we need anything in addition to that.
There was a problem hiding this comment.
could you merge the pr? I cant (I assume because of the downstream tests)
There was a problem hiding this comment.
Huh weird, I expected you to be able to merge it either way. I added it to the merge queue now
There was a problem hiding this comment.
Could you merge it again? I just rebased and fixed a new usage of workspace.ResourcePlugin that made the lint fail
glena
force-pushed
the
german/refactor-pluginkind
branch
from
f50bbaf
to
3ea51c5
Compare
| LanguagePlugin = apitype.LanguagePlugin | ||
| ResourcePlugin = apitype.ResourcePlugin | ||
| ConverterPlugin = apitype.ConverterPlugin | ||
| ToolPlugin = apitype.ToolPlugin | ||
| ) | ||
|
|
||
| // IsPluginKind returns true if k is a valid plugin kind, and false otherwise. | ||
| func IsPluginKind(k string) bool { |
There was a problem hiding this comment.
Feels maybe a little bit odd not to have this in apitype as well, but I don't think it's worth moving unless we actually have to.
There was a problem hiding this comment.
it makes sense I just moved it too
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
glena
force-pushed
the
german/refactor-pluginkind
branch
from
bf380a6
to
9214bef
Compare
Will wait to merge this until after #16057 merges ### Features - [auto/{go,nodejs,python}] Add support for the continue-on-error parameter of the up command to the Automation API [#15953](#15953) - [engine] Add a --continue-on-error flag to pulumi up [#15740](#15740) ### Bug Fixes - [sdk/nodejs] Fix a race condition that could cause the NodeJS runtime to terminate before finishing all work [#16005](#16005) - [sdk/python] Fix an exception when setting providers resource option with a dict [#16022](#16022) - [sdk/python] Fix event loop tracking in the python SDK when using remote transforms [#16039](#16039) - [sdk/python] Workaround lazy module loading regression [#16038](#16038) ### Miscellaneous - [cli/plugin] Move PluginKind type definition into apitype and re-export for backward compatibility [#15946](#15946)
Description
This PR moves PluginKind to apitype to prevent circular dependencies when adding apitype as a dependency of the workspace module.
It also re-exports PluginKind to keep backward compatibility
Checklist
make tidyto update any new dependenciesmake lintto verify my code passes the lint checkgofumptmake changelogand committed thechangelog/pending/<file>documenting my change