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 split Go SDK #1934

Merged
merged 19 commits into from Sep 13, 2022
Merged

Add split Go SDK #1934

merged 19 commits into from Sep 13, 2022

Conversation

danielrbradley
Copy link
Member

@danielrbradley danielrbradley commented Aug 24, 2022

  1. Generate secondary go SDK into sdk/pulumi-azure-native-sdk to match new pulumi-azure-native-sdk repository
  2. Insert root and leaf go.mod files as part of codegen (based on directories with init.go files).
  3. Include replace directives so the modules can be used locally.
  4. Incude current version number as part of go.mod references
    • Fall back to latest if not available.
    • Strip build number from codegen version to make compatible with go module versioning.
  5. Build each go module using find.
  6. Add go_prepublish target to strip go.sum files and go.mod replace directives
  7. Adapt GitHub workflows to generate and persist new go SDK between steps
  8. During publish, checkout new repository in sub-folder then tag and push

Outstanding issues:

Original investigation notes

Investigation into solving #1930 as a proof of concept and to decide a route forward.

Either approach below would be best combined with committing the new go SDK into a new repo such as pulumi-azure-sdk as ~200 tags will need to be added on each release, and it adds significantly more code.

Add go.mod files to the original structure

Pros: The core generation is almost identical, just with extra files appended

Have to reference the root sdk module of the same version. This would require publishing the root SDK first with the new version number, then pulling in the new version number into the per-module go.mod then publishing each after.

We really need the provider to be included in each sub-module directly to remove any internal dependencies between modules that have to be published together.

Split the schema and then generate independently

This results in every module having its own provider class (and config). This doesn't affect when using the default provider, but would look a little more messy with explicit providers.

Split by modifying discovery glob - makes discovery really quick for single modules.

Go package name is calculated from end of importBasePath in go language config.

authorization, storate and synapse seem to be mixins - not from specs. Could add options to skip adding these, have just filtered them out by token for now.

Generate the go.mod and add to list of files to emit.

Optional: Remove Double Nesting

Go module is at something like github.com/pulumi/pulumi-azure-native-sdk/compute. Codegen wants to create a second compute folder within that, but that's not great to have to import github.com/pulumi/pulumi-azure-native-sdk/compute/compute.

How do we avoid the double nesting of module name in Go when there's only one module? Could manually change the paths for now. Should we just live with double nesting for now?

There's separate init files in the root and module folders. The root file has construct provider and registers resource package. The module file has the resource "construct" method and registers resource module. Seems like we can just rename the module init file on moving, then fix the import used for the PkgVersion.

Opportunities to improve core codegen

  • Add option to generate go.mod
  • Allow option for a "default" module to be pulled to the top-level rather than every module having its own folder.

Out of scope

  • Integration of the new Go SDK for ProgramTest - could continue to generate the monolithic SDK temporarily for testing.
  • Publishing workflow - would need to add steps to commit to a separate repo and add relevant tags.

@danielrbradley danielrbradley self-assigned this Aug 24, 2022
@danielrbradley danielrbradley changed the title Danielrbradley/sdk split Investigate feasibility of splitting Go SDK Aug 24, 2022
@Frassle
Copy link
Member

Frassle commented Aug 25, 2022

Add option to generate go.mod

I think we need that in platform for some things anyway. We want to generate some types into the core sdk, but just the code files none of the project related stuff (i.e. no .csproj, go.mod, etc). I think that option would work for you here as well.

@danielrbradley danielrbradley force-pushed the danielrbradley/sdk-split branch 3 times, most recently from 094a376 to b15bdc8 Compare August 31, 2022 10:26
@pulumi pulumi deleted a comment from github-actions bot Aug 31, 2022
@pulumi pulumi deleted a comment from github-actions bot Aug 31, 2022
@danielrbradley danielrbradley changed the title Investigate feasibility of splitting Go SDK Add split Go SDK Aug 31, 2022
@danielrbradley danielrbradley marked this pull request as ready for review August 31, 2022 11:32
@danielrbradley danielrbradley force-pushed the danielrbradley/sdk-split branch 2 times, most recently from 8c665a9 to 5c3076e Compare August 31, 2022 11:42
Copy link
Member Author

@danielrbradley danielrbradley left a comment

Choose a reason for hiding this comment

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

Pair review with @thomas11

Document & changelog

provider/cmd/pulumi-gen-azure-native/main.go Outdated Show resolved Hide resolved
provider/cmd/pulumi-gen-azure-native/main.go Outdated Show resolved Hide resolved
provider/cmd/pulumi-gen-azure-native/main.go Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
@danielrbradley danielrbradley force-pushed the danielrbradley/sdk-split branch 5 times, most recently from d4aa065 to b2de4d5 Compare September 2, 2022 19:12
@pulumi pulumi deleted a comment from github-actions bot Sep 6, 2022
@pulumi pulumi deleted a comment from github-actions bot Sep 6, 2022
@pulumi pulumi deleted a comment from github-actions bot Sep 6, 2022
@pulumi pulumi deleted a comment from github-actions bot Sep 6, 2022
@pulumi pulumi deleted a comment from github-actions bot Sep 6, 2022
@pulumi pulumi deleted a comment from github-actions bot Sep 6, 2022
@pulumi pulumi deleted a comment from github-actions bot Sep 6, 2022
@pulumi pulumi deleted a comment from github-actions bot Sep 6, 2022
@pulumi pulumi deleted a comment from github-actions bot Sep 6, 2022
@pulumi pulumi deleted a comment from github-actions bot Sep 6, 2022
@danielrbradley danielrbradley added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Sep 8, 2022
- Maintain the commit hash from the build of the version number.
- Add simple unit test suite of examples.
- Output version to a file in the go SDK.
- Add readme to the Go SDK.
- When publishing Go, use the version from the version.txt to keep it all aligned.
@github-actions
Copy link

github-actions bot commented Sep 8, 2022

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.
Looking good! No API changes found.

*Fix to use US spelling
@pulumi pulumi deleted a comment from github-actions bot Sep 9, 2022
@pulumi pulumi deleted a comment from github-actions bot Sep 9, 2022
@pulumi pulumi deleted a comment from github-actions bot Sep 9, 2022
@pulumi pulumi deleted a comment from github-actions bot Sep 9, 2022
@pulumi pulumi deleted a comment from github-actions bot Sep 9, 2022
@pulumi pulumi deleted a comment from github-actions bot Sep 9, 2022
Copy link
Contributor

@viveklak viveklak left a comment

Choose a reason for hiding this comment

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

Should/can we add an acceptance test for this?

Makefile Show resolved Hide resolved

require (
github.com/blang/semver v3.5.1+incompatible
github.com/pkg/errors v0.9.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't realize we were still depending on pkg/errors...

Copy link
Contributor

Choose a reason for hiding this comment

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

One general question would be how we update this template if versions change or we need additional dependencies added etc. Do you have thoughts on documenting a dev loop for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea on what we actually use. This is taken from our currently manually committed go.mod in the sdk folder which we currently manually maintain. I can add something to the contributing docs around updating this. Long term, I think codegen should be generating these dependencies - so they can be upgraded based on what the generated code will need - similar in approach to this issue around Python dependencies

Copy link
Member Author

Choose a reason for hiding this comment

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

Just checked and apparently we do actually use that package in the generated SDK.

@danielrbradley
Copy link
Member Author

Should/can we add an acceptance test for this?

So, not entirely straightforward to figure out, but it seems like it should work!

@pulumi pulumi deleted a comment from github-actions bot Sep 12, 2022
@pulumi pulumi deleted a comment from github-actions bot Sep 12, 2022
@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.
Looking good! No API changes found.

@pulumi pulumi deleted a comment from github-actions bot Sep 12, 2022
@danielrbradley
Copy link
Member Author

@viveklak doesn't look like program tests are likely to work at the time. Worth just doing a quiet release alongside the old SDK so we can continue testing against an actual real release?

@danielrbradley danielrbradley merged commit d18468f into master Sep 13, 2022
@pulumi-bot pulumi-bot deleted the danielrbradley/sdk-split branch September 13, 2022 08:24
@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.
Looking good! No API changes found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/no-changelog-required This issue doesn't require a CHANGELOG update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants