-
Notifications
You must be signed in to change notification settings - Fork 9.9k
Add stacks plugin proto #36931
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
base: main
Are you sure you want to change the base?
Add stacks plugin proto #36931
Conversation
Changelog WarningCurrently this PR would target a v1.13 release. Please add a changelog entry for in the .changes/v1.13 folder, or discuss which release you'd like to target with your reviewer. If you believe this change does not need a changelog entry, please add the 'no-changelog-needed' label. |
add tests update changelog
03462d8
to
3bc1359
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smoke testing is looking good from the perspective of the Stacks plugin 🚀 ! Mostly just one point about about an additional bit of needed info in the StacksPluginConfig
Otherwise I'm realizing that we aren't fully honoring the -json
flag here when we have diagnostics returned. Though it's an awkward situation since not all stacks
subcommands implement a json view. Don't think it's a massive problem since we'd only return diags here if the hostname/token env vars aren't configured, or if the plugin dev override is in use. Wanted to call that out, but I don't have a strong opinion on how/if to handle it.
Found out the cause (and the fixes) of the occasional nil panics we were seeing in the prototype, stemming from services here not being properly managed. One note is that in addition to what we were seeing in Accessing the service before it's fully set upIn This looks to be a bit of a race condition since we start the services within asynchronous go routines: dependenciesBrokerID := c.Broker.NextId()
go c.Broker.AcceptAndServe(dependenciesBrokerID, dependenciesServerFunc)
packagesBrokerID := c.Broker.NextId()
go c.Broker.AcceptAndServe(packagesBrokerID, packagesServerFunc)
stacksBrokerID := c.Broker.NextId()
go c.Broker.AcceptAndServe(stacksBrokerID, stacksServerFunc) but SolutionImplement a mechanism to ensure all broker services are fully initialized before proceeding: func (c GRPCStacksClient) registerBrokers(stdout, stderr io.Writer) brokerIDs {
// ... existing code ...
// Create channels to signal when each service is ready
dependenciesReady := make(chan struct{})
packagesReady := make(chan struct{})
stacksReady := make(chan struct{})
dependenciesServerFunc := func(opts []grpc.ServerOption) *grpc.Server {
s = grpc.NewServer(opts...)
dependencies.RegisterDependenciesServer(s, dependenciesServer)
dependenciesServer.ActivateRPCServer(newDependenciesServer(handles, c.Services))
close(dependenciesReady) // Signal that this service is ready
return s
}
// Similar modifications for other services...
// Wait for all services to be ready
<-dependenciesReady
<-packagesReady
<-stacksReady
return brokerIDs{...}
} The service closing before the complete execution of the commandThis was the class of problem we were seeing in the In SolutionGet rid of the dependenciesServerFunc := func(opts []grpc.ServerOption) *grpc.Server {
s := grpc.NewServer(opts...) // the := creating a new variable
// ... other code ...
return s
} |
ctx: ctx, | ||
serviceURL: serviceURL, | ||
httpClient: retryableClient, | ||
pluginName: "cloudplugin", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you are scrubbing the term "cloudplugin" from this package, which I think is a good idea. But it's not clear to me if you went as far as you could go or if you just missed a few of them. This is just one example.
IMO, it's OK to remove or manipulate all cloudplugin features from terraform and shift those over to stacks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went about the rearrangement by creating a base client
so that each plugin could use an instance of the base client, that's why there is still both cloudclient
and stacksclient
in the package.
Fixes #
terraform stacks
terraform stacks
exposes some Terraform Stack operations through the cli. The available subcommands depend on the stacks plugin implementation.NOTE: the
terraform stacks -help
will be implemented in a separate PRTarget Release
1.13.x
CHANGELOG entry
The new command
terraform stacks
exposes some Terraform Stack operations through the cli. The available subcommands depend on the stacks plugin implementation. Useterraform stacks -help
to see available commands.This change is user-facing and I added a changelog entry.
This change is not user-facing.
NOTE: Test steps are written in the project slack channel