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

Fix default package name #8187

Merged

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented Oct 11, 2021

Resulting behavior:

name importBasePath rootPackageName resulting root resulting name
hello-world hello-world helloworld
hello-world github.com/org/my-pulumi-package-aws-thing/my-thing mything mything
hello-world github.com/org/my-pulumi-package-aws-thing/thing bar bar
github.com/org/my-pulumi-package-aws-thing/thing bar bar
hello-world bar bar
bar bar

Description

Corrects the default behavior for deriving package names from the name, importBasePath and rootPackageName.

Fixes #8181

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

| name          | importBasePath                                        | rootPackageName | resulting root | resulting name                    |
|:-------------:|:-----------------------------------------------------:|:---------------:|----------------|:---------------------------------:|
| `hello-world` | ` `                                                   | ` `             | `hello-world`  | `helloworld`                      |
| `hello-world` | `github.com/org/my-pulumi-package-aws-thing/my-thing` | ` `             | `mything`      | `mything`                         |
| `hello-world` | `github.com/org/my-pulumi-package-aws-thing/thing`    | `bar`           | ` `            | `bar` (and the package is flat)   |
| ` `           | `github.com/org/my-pulumi-package-aws-thing/thing`    | `bar`           | ` `            | `bar`   (and the package is flat) |
| `hello-world` | ` `                                                   | `bar`           | ` `            | `bar`  (and the package is flat)  |
| ` `           | ` `                                                   | `bar`           | ` `            | `bar`  (and the package is flat)  |
@iwahbe iwahbe self-assigned this Oct 11, 2021
@iwahbe iwahbe changed the title Fix default pacakge name Fix default package name Oct 11, 2021
@github-actions
Copy link

Diff for pulumi-azuread with merge commit 98cef56

@github-actions
Copy link

Diff for pulumi-random with merge commit 98cef56

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 72f9d34

@github-actions
Copy link

Diff for pulumi-kubernetes with merge commit 98cef56

@github-actions
Copy link

Diff for pulumi-random with merge commit 72f9d34

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 98cef56

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 72f9d34

@github-actions
Copy link

Diff for pulumi-kubernetes with merge commit 72f9d34

@github-actions
Copy link

Diff for pulumi-aws with merge commit 98cef56

@github-actions
Copy link

Diff for pulumi-azure with merge commit 98cef56

@github-actions
Copy link

Diff for pulumi-aws with merge commit 72f9d34

@github-actions
Copy link

Diff for pulumi-azure with merge commit 72f9d34

@github-actions
Copy link

Diff for pulumi-azuread with merge commit d2118f0

@github-actions
Copy link

Diff for pulumi-random with merge commit d2118f0

@github-actions
Copy link

Diff for pulumi-kubernetes with merge commit d2118f0

@github-actions
Copy link

Diff for pulumi-gcp with merge commit d2118f0

@github-actions
Copy link

Diff for pulumi-azure with merge commit d2118f0

@github-actions
Copy link

Diff for pulumi-aws with merge commit d2118f0

@github-actions
Copy link

Diff for pulumi-azure-native with merge commit d2118f0

@iwahbe iwahbe requested a review from a team October 11, 2021 21:25
@@ -50,8 +50,8 @@ func TestInputUsage(t *testing.T) {

func TestGoPackageName(t *testing.T) {
assert.Equal(t, "aws", goPackage("aws"))
assert.Equal(t, "azure", goPackage("azure-nextgen"))
assert.Equal(t, "plant", goPackage("plant-provider"))
assert.Equal(t, "azurenextgen", goPackage("azure-nextgen"))
Copy link
Member

@komalali komalali Oct 11, 2021

Choose a reason for hiding this comment

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

Do we know what effect (if any) this change will have on azure-native and aws-native?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like they don't include the -native anyway:

"go": {
    "importBasePath": "github.com/pulumi/pulumi-aws-native/sdk/go/aws",
    "packageImportAliases": {
        "github.com/pulumi/pulumi-aws-native/sdk/go/aws/aws-native": "aws"
    }
},

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe that packageImportAliases went unused. We defaulted to getting the name from the name field, and we took strings.Split("aws-native", "-")[0] == "aws". In the new world we take path.Base("github.com/pulumi/pulumi-aws-native/sdk/go/aws") == "aws". This is the same as for azure.

Copy link
Member

@justinvp justinvp left a comment

Choose a reason for hiding this comment

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

LGTM

@iwahbe iwahbe merged commit 28528ba into master Oct 11, 2021
@pulumi-bot pulumi-bot deleted the iwahbe/8181/go-improve-hyphenated-package-name-handling branch October 11, 2021 22:28
@komalali
Copy link
Member

@iwahbe I'm not sure this is going to work all the way through codegen. I just tried changing the importBasepPath in https://github.com/pulumi/pulumi/blob/master/pkg/codegen/internal/test/testdata/simple-enum-schema/schema.json#L250 to "simple-enum-schema/plant-provider" and I see a bunch of compilation errors. Am I missing something?

@iwahbe
Copy link
Member Author

iwahbe commented Oct 11, 2021

We assume that the last segment of importBasePath is a valid go identifier. This results in invalid code generated from paths like "simple-enum-schema/plant-provider".

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 codegen doesn't handle hyphenated package names
3 participants