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

Go program gen: resource range, readDir, template strings, etc #4818

Merged
merged 4 commits into from
Jun 16, 2020

Conversation

EvanBoyle
Copy link
Contributor

@EvanBoyle EvanBoyle commented Jun 13, 2020

Everything needed to support the s3-folder example. Also added a step to build a set of scope traversal tools allowing later replacement of unused variable declarations with _.
Fixes #4781
Fixes #4771

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 3e145eb

@github-actions
Copy link

Diff for pulumi-random with merge commit 3e145eb

@github-actions
Copy link

Diff for pulumi-kubernetes with merge commit 3e145eb

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 3e145eb

@github-actions
Copy link

Diff for pulumi-kubernetes with merge commit db3de5b

@github-actions
Copy link

Diff for pulumi-random with merge commit db3de5b

@github-actions
Copy link

Diff for pulumi-azuread with merge commit db3de5b

@github-actions
Copy link

Diff for pulumi-gcp with merge commit db3de5b

@github-actions
Copy link

Diff for pulumi-aws with merge commit 3e145eb

@github-actions
Copy link

Diff for pulumi-azure with merge commit 3e145eb

@github-actions
Copy link

Diff for pulumi-azure with merge commit db3de5b

@github-actions
Copy link

Diff for pulumi-aws with merge commit db3de5b

@github-actions
Copy link

Diff for pulumi-azuread with merge commit e895e86

@github-actions
Copy link

Diff for pulumi-random with merge commit e895e86

@github-actions
Copy link

Diff for pulumi-kubernetes with merge commit e895e86

@github-actions
Copy link

Diff for pulumi-gcp with merge commit e895e86

@github-actions
Copy link

Diff for pulumi-aws with merge commit e895e86

@github-actions
Copy link

Diff for pulumi-azure with merge commit e895e86

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 1d317f9

@github-actions
Copy link

Diff for pulumi-kubernetes with merge commit 1d317f9

@github-actions
Copy link

Diff for pulumi-random with merge commit 1d317f9

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 1d317f9

@github-actions
Copy link

Diff for pulumi-azure with merge commit 1d317f9

@github-actions
Copy link

Diff for pulumi-aws with merge commit 1d317f9

@@ -608,10 +629,11 @@ func (nameInfo) Format(name string) string {
// lowerExpression amends the expression with intrinsics for C# generation.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// lowerExpression amends the expression with intrinsics for C# generation.
// lowerExpression amends the expression with intrinsics for Go generation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix this in my next PR

_, err = s3.NewBucketObject(ctx, "files-"+string(i0), &s3.BucketObjectArgs{
Bucket: siteBucket.ID(),
Key: pulumi.String(val0),
Source: pulumi.NewFileAsset(fmt.Sprintf("%v%v%v", siteDir, "/", val0)),
Copy link
Member

Choose a reason for hiding this comment

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

I realize that this is a result of the input program, but paths should really use path.Join instead of a hardcoded slash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, but there isn't really a good way to handle this (at least in terms of a sensible effort/reward ratio). The other language generators do the same.

@EvanBoyle
Copy link
Contributor Author

Merging this to get the next PR open. Happy to address any late feedback in a new changeset.

@EvanBoyle EvanBoyle merged commit 2d61852 into master Jun 16, 2020
@pulumi-bot pulumi-bot deleted the evan/MoreGoProgramGen branch June 16, 2020 06:00
Copy link
Member

@pgavlin pgavlin left a comment

Choose a reason for hiding this comment

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

Sorry for the late feedback--LGTM overall.

pkg/codegen/go/gen_program.go Show resolved Hide resolved
@@ -202,9 +267,32 @@ func (g *generator) genTemps(w io.Writer, temps []interface{}) {
args := stripInputs(t.Value.Args[0])
g.Fgenf(w, "%.v)\n", args)
g.Fgenf(w, "if err != nil {\n")
g.Fgenf(w, "return err\n")
if genZeroValueDecl {
g.Fgenf(w, "return _zero, err\n")
Copy link
Member

Choose a reason for hiding this comment

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

Another way to do this is *new(T), which is maybe a bit more obscure, but has the benefit of avoiding the variable declaration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, a good tip for the tool belt. I think it might be favorable to avoid obscure syntax for code snippets and docs. Will leave as is unless you have other objections.

pkg/codegen/go/gen_program_expressions.go Show resolved Hide resolved
pkg/codegen/go/gen_program.go Show resolved Hide resolved
Comment on lines +12 to +15
type readDirTemp struct {
Name string
Value *model.FunctionCallExpression
}
Copy link
Member

Choose a reason for hiding this comment

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

We should think about unifying these temp kinds in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With what goal? Eliminating redundancy in the spillers?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's right.

@EvanBoyle
Copy link
Contributor Author

@pgavlin thanks for taking a look. I pushed the suggested changes into my latest PR: #4831

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.

[Go Program Gen] Passing test for aws-s3-folder.pp [Go Program Gen] convert unused variables to _
3 participants