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

Refactor controller to use Automation API #86

Merged
merged 20 commits into from
Sep 16, 2020
Merged

Conversation

metral
Copy link
Contributor

@metral metral commented Aug 27, 2020

Proposed changes

Refactor controller to use Automation API

Related issues (optional)

Closes #85

@metral metral force-pushed the metral/automation-api branch 11 times, most recently from 4ba8f90 to 7c236b0 Compare September 3, 2020 19:25
@metral metral marked this pull request as ready for review September 9, 2020 16:47
Copy link
Member

@lblackstone lblackstone left a comment

Choose a reason for hiding this comment

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

Looking good overall! Since it looks like the automation API doesn't support a few existing features yet, do we want to hold off on this, or is it better to move ahead with the refactor?

Dockerfile Outdated Show resolved Hide resolved
pkg/controller/stack/stack_controller.go Outdated Show resolved Hide resolved
pkg/controller/stack/stack_controller.go Outdated Show resolved Hide resolved
pkg/controller/stack/stack_controller.go Outdated Show resolved Hide resolved
@EvanBoyle
Copy link

I believe the secrets provider work is the last big TODO for automation API and we should have feature parity after that. I'm not opposed to holding off until that lands.

@metral
Copy link
Contributor Author

metral commented Sep 9, 2020

do we want to hold off on this, or is it better to move ahead with the refactor?

I'm not opposed to holding off until that lands.

Thanks for the review! Holding off SGTM 👍

@metral metral force-pushed the metral/automation-api branch 6 times, most recently from b7711c8 to 2099e99 Compare September 15, 2020 00:02
@metral metral force-pushed the metral/automation-api branch from 2099e99 to 9b45f15 Compare September 15, 2020 18:54
@metral metral requested review from lblackstone and EvanBoyle and removed request for EvanBoyle September 15, 2020 18:54
Copy link
Member

@lblackstone lblackstone left a comment

Choose a reason for hiding this comment

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

🎉 Looks awesome!

Copy link

@EvanBoyle EvanBoyle left a comment

Choose a reason for hiding this comment

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

This looks great! Awesome to see so much red! 🚀

One thing I noticed was that there is a small regression in functionality. Previously pulumi up and company were being run through the runCmd func which has it's own io handling to pipe stdout to logs that can be followed by kubectl and such:

stdoutR, stdoutW := io.Pipe()
stderrR, stderrW := io.Pipe()
cmd.Stdout = stdoutW
cmd.Stderr = stderrW
// Start the command asynchronously.
err := cmd.Start()
if err != nil {
return "", "", err
}
// Kick off some goroutines to stream the output asynchronously. Since Pulumi can take
// a while to run, this helps to debug issues that might be ongoing before a command completes.
var stdout bytes.Buffer
var stderr bytes.Buffer
go func() {
outs := bufio.NewScanner(stdoutR)
for outs.Scan() {
text := outs.Text()
sess.logger.Info(title, "Path", cmd.Path, "Args", cmd.Args, "Stdout", text)
stdout.WriteString(text + "\n")
}
}()
go func() {
errs := bufio.NewScanner(stderrR)
for errs.Scan() {
text := errs.Text()
sess.logger.Info(title, "Path", cmd.Path, "Args", cmd.Args, "Text", text)
stderr.WriteString(text + "\n")
}
}()

Given that updates are run through automation API, those logs won't flow anymore. A solution here is to wire up ProgressStreams from this PR pulumi/pulumi#5367 for up/refresh/destroy/preview with the io.Writer from the logging interface (not sure if that's a logging interface or if it just goes to stdout). Not blocking, but something to consider.

pkg/controller/stack/stack_controller.go Outdated Show resolved Hide resolved
pkg/controller/stack/stack_controller.go Outdated Show resolved Hide resolved
@metral
Copy link
Contributor Author

metral commented Sep 15, 2020

Opened #87 to track the use of ProgressStreams for logs.

@metral metral merged commit 54a7432 into master Sep 16, 2020
@metral metral deleted the metral/automation-api branch September 16, 2020 01:49
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.

Refactor controller to use Automation API
3 participants