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

Add Image Support for Go #147

Merged
merged 19 commits into from
Mar 12, 2020
Merged

Add Image Support for Go #147

merged 19 commits into from
Mar 12, 2020

Conversation

tasiah
Copy link

@tasiah tasiah commented Mar 6, 2020

Fixes #128.

A couple of things to note:

Copy link
Contributor

@lukehoban lukehoban left a comment

Choose a reason for hiding this comment

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

Looks great! 🎉

sdk/go/docker/image.go Outdated Show resolved Hide resolved
sdk/go/docker/image.go Outdated Show resolved Hide resolved
sdk/go/docker/image.go Outdated Show resolved Hide resolved
sdk/go/docker/image.go Outdated Show resolved Hide resolved
sdk/go/docker/image.go Outdated Show resolved Hide resolved
sdk/go/docker/docker.go Outdated Show resolved Hide resolved
sdk/go/docker/docker.go Outdated Show resolved Hide resolved
sdk/go/docker/docker.go Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

Looks good to me. Most of the comments are more from the perspective of what we could do with the platform to improve the authorship/user experience for building something like this.

Seems like we're doing a lot of string building, manipulation, and some matching/parsing that would be easy to unit test. Not blocking, but something to consider.

"github.com/pulumi/pulumi/sdk/go/pulumi"
)

type CacheFrom struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a meta-comment on our current requirements for user-defined types: I think that the level of complexity is too high for most users to be successful here even with proper documentation. Is there something we can do here in terms of tooling to improve this authoring experience? Could we do something with templates pulumi new, or potentially provide code generation tools for some of this?

It seems like the user only really needs to synthesize T and TArgs structs, and everything else is boilerplate.

Thoughts @pgavlin ?

Copy link
Member

Choose a reason for hiding this comment

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

This does get more complicated with nested types. I'd love to avoid adding yet another code generator, but if we did, we'd probably do something like

//+pulumi
type Foo struct {
}

And then let a code generator chew on that. We might be able to do this by refactoring the schema generator a bit and adding a frontend that works off of parsed code.

At the moment, though, only the structs with input-typed fields should be necessary, and they shouldn't need to implement Input. The result will be a bit clumsy to work with--it will need type assertions--but it does avoid a fair bit of hand-written code.

sdk/go/docker/customTypes.go Show resolved Hide resolved
Target string `pulumi:"target"`
}

// DockerBuild may be used to specify detailed instructions about how to build a container.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Doc comments start with the name of the type/function/var in question.

Another thing I've noticed is that users sometimes get quite confused about which form of T TArgs TOutput TInput to use in various situations. This comment from Cameron in slack illustrates my point:

Given the type definition EventSelectors TrailEventSelectorArrayInput - how do I construct that? 

I wonder if we can come up with some sort of standard boilerplate comment for each type so that their intended usage is a little easier to understand.

For instance:

// T may be used to specify detailed instructions about how to build a container.
// T is the raw representation of the type.

// TArgs is the pulumified version of T where are members are outputs. Construct TArgs as an input to NewDockerBuild(...)

@lukehoban @pgavlin thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

For some more detail on the above example:

https://github.com/pulumi/pulumi-aws/blob/c29dcf1b7f3209874af908b4fa94c81991b5adde/sdk/go/aws/cloudtrail/trail.go#L155

If you go searching for that type, it's a little daunting to try and figure out which one to pick if you aren't intimately familiar with our go type system:

https://github.com/pulumi/pulumi-aws/blob/c29dcf1b7f3209874af908b4fa94c81991b5adde/sdk/go/aws/cloudtrail/pulumiTypes.go

Copy link
Author

Choose a reason for hiding this comment

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

Another thing I've noticed is that users sometimes get quite confused about which form of T TArgs TOutput TInput to use in various situations.

+1 Because I was/am one of those users.

Copy link
Member

Choose a reason for hiding this comment

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

Coming up with a standard comment here would be fantastic, yes.

sdk/go/docker/docker.go Show resolved Hide resolved
sdk/go/docker/docker.go Outdated Show resolved Hide resolved
sdk/go/docker/docker.go Outdated Show resolved Hide resolved
sdk/go/docker/docker.go Show resolved Hide resolved
@tasiah tasiah merged commit 889285d into master Mar 12, 2020
@pulumi-bot pulumi-bot deleted the tasiah/RunCommandThatMustSucceed branch March 12, 2020 21:13
@lukehoban
Copy link
Contributor

🎉

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.

4 participants