-
Notifications
You must be signed in to change notification settings - Fork 8
Switching to new SDK provider tool for makefiles and adding to mise #1822
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
base: master
Are you sure you want to change the base?
Conversation
| entries := pluginsToToolEntries(pluginList) | ||
|
|
||
| //Manually adding the new SDK tool to this list until we support dynamically adding new dependencies | ||
| sdkToolPlugin := sectionEntry{"go:github.com/pulumi/provider-sdk-builder", "latest"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
long term we'll probably want to switch to using a binary which is published on the GitHub release.
"github:pulumi/provider-sdk-builder" = "latest"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do that. I would just need to get the binaries getting built and included on the release. Any reason we prefer the binaries to go installation though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we will run into less issues long term by using a built binary. This method uses go install which can lead to issues. https://golangci-lint.run/docs/welcome/install/#install-from-sources
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
golangci-lint is a bit of an odd duck and that guidance isn't true in general. Sticking with go install should be fine for something this simple.
| @touch $@ | ||
|
|
||
| # Build the provider and all SDKs and install ready for testing | ||
| build: .make/mise_install provider build_sdks install_sdks#{{- if .Config.RegistryDocs }}# build_registry_docs#{{- end }}# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming that install is now run as part of build in the tool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the latest version I actually have changed this. You can pass in an SDK to build. It is run as part of the build in root though also
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you speak to this? Does build include install or not?
| install_sdks:#{{ range .Config.Languages }}# install_#{{ . }}#_sdk#{{ end }}# | ||
| .PHONY: development only_build build generate generate_sdks build_sdks install_sdks mise_install mise_env | ||
| # Creates all generated files of which the schema is committed | ||
| generate: build_sdks schema#{{- if .Config.RegistryDocs }}# build_registry_docs#{{- end }}# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we still generating the SDKs? If so I think we should still separate out the generate from build. Building the sdks can take a while and it's nice to not have to run it if you don't have to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried to solve that problem by only triggering a build when the schema has changed. I am anticipating we will still always generate them once (at least in this current world)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm more suggesting that when iterating I might just want to regenerate the SDKs and not build the SDKs. But that's probably pretty uncommon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you can do that by invoking the build tool directly, but I also thought that was uncommon and didn't feel it needed to be a target in the makefile. Happy to change if you feel strongly though!
| @echo " generate_sdks Generate all SDKs" | ||
| @echo " build_sdks Build all SDKs" | ||
| @echo " install_sdks Install all SDKs" | ||
| @echo " provider_dist Build and package the provider for all platforms" | ||
| @echo "" | ||
| @echo "Tool Targets" | ||
| @echo " ci-mgmt Re-generate CI configuration from .ci-mgmt.yaml" | ||
| @echo " debug_tfgen Start a debug server for tfgen" | ||
| @echo "" | ||
| @echo "Internal Targets (automatically run as dependencies of other targets)" | ||
| @echo " prepare_local_workspace Prepare for building" | ||
| @echo " mise_install Install tools with mise" | ||
| @echo " upstream Initialize the upstream submodule, if present" | ||
| @echo "" | ||
| @echo "Language-Specific Targets" | ||
| @echo " generate_[language] Generate the SDK files ready for committing" | ||
| @echo " build_[language] Build the SDK to check correctness" | ||
| @echo " install_[language]_sdk Install the SDK ready for testing" | ||
| @echo "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we still need all these targets? If I'm running locally and just want the nodejs SDKs installed I can just run the language specific targets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the new build target also runs install then we probably don't need this anymore now that we have the SDK_LANG
Removing step to install go and allowing mise to handle installation Updating to support new SDK generation tool updating download SDK action to create directory if missing Updating the pulumi package publish workflows adding provider version to new SDKs and updating the non java publish workflow to use the latest provider publisher commit Going to download the SDKs before the verify step since they are no longer mastered in source control
…e in the sdk directory Switching the build SDK workflow to use the pulumi bot token switching install to use the sdk tool directly instead of a make target
8bcd4fa to
079e7b0
Compare
| - name: Setup Python | ||
| uses: actions/setup-python@83679a892e2d95755f2dac6acb0bfd1e9ac5d548 # v6 | ||
| with: | ||
| python-version: 3.9 | ||
| - name: Install pulumictl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the other steps can be removed if we're installing with mise now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will rip all these out and retest.
| "aqua:gradle/gradle-distributions" = '7.6.6' | ||
| golangci-lint = "1.64.8" # See note about about overrides if you need to customize this. | ||
| "npm:yarn" = "1.22.22" | ||
| "go:github.com/pulumi/provider-sdk-builder" = "latest" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should pin this if it's going to be going out everywhere.
| //Manually adding the new SDK tool to this list until we support dynamically adding new dependencies | ||
| sdkToolPlugin := sectionEntry{"go:github.com/pulumi/provider-sdk-builder", "latest"} | ||
| entries = append(entries, sdkToolPlugin) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need this, the mise.toml in this directory should already capture the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was testing off an old branch of this package, but I know we did need it for a time. Repos that already had a mise.toml file to add it to the existing ones.
Let me rip it out and check when I retest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this file has changed a lot since I wrote that code, because I had like 10 sets of merge conflicts in here when I was rebasing my changes onto mainline haha
| with: | ||
| version: 2025.11.6 | ||
| github_token: ${{ steps.esc-secrets.outputs.PULUMI_BOT_TOKEN }} | ||
| cache_key: "mise-{{platform}}-{{file_hash}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be using the default cache key everywhere now.
| cache_key: "mise-{{platform}}-{{file_hash}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack
| #{{- end }}# | ||
| #{{- if .Config.IntegrationTestProvider }}# | ||
| - name: Install dependencies | ||
| run: provider-sdk-builder install --sdkLocation ./sdk/${{ matrix.language}} -l ${{ matrix.language}} --installLocation ${{ github.workspace }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more of a nit but is this step less of an "install" and more of a "link local SDK"?
Also formatting
| run: provider-sdk-builder install --sdkLocation ./sdk/${{ matrix.language}} -l ${{ matrix.language}} --installLocation ${{ github.workspace }} | |
| run: provider-sdk-builder install --sdkLocation ./sdk/${{ matrix.language }} -l ${{ matrix.language }} --installLocation ${{ github.workspace }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to make those changes, and yes that was probably not the correct name
| language: ${{ matrix.language }} | ||
| - name: Restore makefile progress | ||
| run: make --touch provider schema build_${{ matrix.language }} | ||
| run: make --touch provider schema build_sdks SDK_LANG=${{ matrix.language }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not invoke provider-sdk-builder directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah thats not a bad idea. I was looking at this and scratching my head right now too, because I don't believe it should even be building the SDK in this step....
I am just going to remove this build_sdk call. I don't think its right to trigger that behavior here, and I think as I side effect of how we use our sentinel files I am not sure it actually was building.... 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is using --touch so it's not actually building. We do this so that later when we run make test it thinks (correctly) that the SDKs have already been built. Without this make test will re-build the SDKs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that makes sense. For some reason I was reading the touch flag as only touching the sentinel file for the first target, but obviously that is not how it works
Thanks for clarifying!
| - name: Download python SDK | ||
| uses: ./.github/actions/download-sdk | ||
| with: | ||
| language: python |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems counter to what the workflow is trying to do, and if we needed to add this step this might point towards a bug somewhere.
In particular I think this workflow is trying to test the end-to-end experience from the user's perspective, which means that it expects to find all the relevant SDKs and binaries published and available from the usual places.
What we're doing here is grabbing an SDK that was generated earlier. Why weren't we able to grab the SDK from npm, pypi, etc.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me take a deeper look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot lie, I have no memory of writing this step anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this what was causing your issue with windows in verify-release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it was one of them yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re: Windows that would make sense. If this isn't needed then we can continue to test Windows in this workflow, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I reviewed and re-tested and these are required for the verify release workflows. What was happening is that the old verify release workflow was relying on the SDKs that use to be checked into source. You can see the failure when it does not have this code in the following workflow when it tries to set up the java SDK to run: https://github.com/pulumi/pulumi-random/actions/runs/19482295415/job/55757193191
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is just a side-effect of how the setup-java action works. It must expect a local java project in certain cases and it was a coincidence that we had the SDKs checked out. I think it might be because the setup-java action tries to restore the gradle cache and looks for local projects. Maybe we can just turn that off or trick it somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think you might be correct. I think Bryce just linked me something to that effect as well. I am going to try to build this with the plugin changes that Bryce made yesterday to see if that unblocks me.
Failing that I can revert this workflow to how it was, and disable that caching.
| - name: Install dependencies | ||
| run: make install_${{ matrix.language}}_sdk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to add this back in but update it to the latest way. Next version will have a change. This is why the XYZ provider fails to build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still outstanding?
| entries := pluginsToToolEntries(pluginList) | ||
|
|
||
| //Manually adding the new SDK tool to this list until we support dynamically adding new dependencies | ||
| sdkToolPlugin := sectionEntry{"go:github.com/pulumi/provider-sdk-builder", "latest"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we will run into less issues long term by using a built binary. This method uses go install which can lead to issues. https://golangci-lint.run/docs/welcome/install/#install-from-sources
| language: ${{ matrix.language }} | ||
| - name: Restore makefile progress | ||
| run: make --touch provider schema build_${{ matrix.language }} | ||
| run: make --touch provider schema build_sdks SDK_LANG=${{ matrix.language }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is using --touch so it's not actually building. We do this so that later when we run make test it thinks (correctly) that the SDKs have already been built. Without this make test will re-build the SDKs.
| - name: Download python SDK | ||
| uses: ./.github/actions/download-sdk | ||
| with: | ||
| language: python |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this what was causing your issue with windows in verify-release?
| install_sdks:#{{ range .Config.Languages }}# install_#{{ . }}#_sdk#{{ end }}# | ||
| .PHONY: development only_build build generate generate_sdks build_sdks install_sdks mise_install mise_env | ||
| # Creates all generated files of which the schema is committed | ||
| generate: build_sdks schema#{{- if .Config.RegistryDocs }}# build_registry_docs#{{- end }}# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm more suggesting that when iterating I might just want to regenerate the SDKs and not build the SDKs. But that's probably pretty uncommon.
| @echo " generate_sdks Generate all SDKs" | ||
| @echo " build_sdks Build all SDKs" | ||
| @echo " install_sdks Install all SDKs" | ||
| @echo " provider_dist Build and package the provider for all platforms" | ||
| @echo "" | ||
| @echo "Tool Targets" | ||
| @echo " ci-mgmt Re-generate CI configuration from .ci-mgmt.yaml" | ||
| @echo " debug_tfgen Start a debug server for tfgen" | ||
| @echo "" | ||
| @echo "Internal Targets (automatically run as dependencies of other targets)" | ||
| @echo " prepare_local_workspace Prepare for building" | ||
| @echo " mise_install Install tools with mise" | ||
| @echo " upstream Initialize the upstream submodule, if present" | ||
| @echo "" | ||
| @echo "Language-Specific Targets" | ||
| @echo " generate_[language] Generate the SDK files ready for committing" | ||
| @echo " build_[language] Build the SDK to check correctness" | ||
| @echo " install_[language]_sdk Install the SDK ready for testing" | ||
| @echo "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the new build target also runs install then we probably don't need this anymore now that we have the SDK_LANG
| shell: bash | ||
| run: tar -zxf ${{ github.workspace }}/sdk/${{ inputs.language }}.tar.gz -C ${{ github.workspace }}/sdk/${{ inputs.language }} | ||
| run: mkdir -p ${{ github.workspace }}/sdk/${{ inputs.language }} && tar -zxf ${{ github.workspace }}/sdk/${{ inputs.language }}.tar.gz -C ${{ github.workspace }}/sdk/${{ inputs.language }} | ||
| - name: Uncompress SDK folder (Windows) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this only needed because of verify-release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used for more steps than just that. Several steps download the SDK. We needed to make this change because tar will fail if the destination folder does not exist without the flag I added. Since the SDK folders are not checked in this is needed for the verify and test workflows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm referring specifically to the (Windows) piece
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yeah that is needed for the verify release windows. I left it (even though I disabled the job) as I do anticipate adding it back in at a later date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the other comment we don't want to download SDKs in the verify-release job since we want to use the published SDKs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind if I tackled that at a different time when I go after the verify-release job?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand it we don't (or shouldn't) need to touch verify-release, so let's roll those changes back before shipping this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good
…ree different versions of it that existed
|
|
||
| install_sdks: .make/install_sdks | ||
| .make/install_sdks: .make/build_sdks | ||
| provider-sdk-builder build-sdks --language $(SDK_LANG) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does provider-sdk-builder build-sdks install too? This seems too similar to .make/build_sdks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You just caught a huge typo! Thank you so much. The 'build-sdks' argument passed needs to be instead be 'install'.
provider-ci/internal/pkg/templates/base/.github/workflows/verify-release.yml
Outdated
Show resolved
Hide resolved
| # dependencies, commit the updated SDK and push it back to the PR. The | ||
| # job will still be marked as a failure. | ||
| if: failure() && steps.worktreeClean.outcome == 'failure' && contains(github.actor, 'renovate') && github.event_name == 'pull_request' | ||
| if: failure() && contains(github.actor, 'renovate') && github.event_name == 'pull_request' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change intentional that all failures (not just dirty worktrees caused by SDK generation) will trigger a commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhh no it was not. My intent was to remove the worktree clean step. Very good catch let me rework it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just ripped this whole step of the workflow out
|
Also enough people have asked for a compile target I will add that back in |
| // If we have any plugins overrides, move them to the .config/mise.toml | ||
| pluginList := nodeToPluginEntries(plugins) | ||
| entries := pluginsToToolEntries(pluginList) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: surprise blank line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some feedback to be addressed.
| - name: Install dependencies | ||
| run: make install_${{ matrix.language}}_sdk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still outstanding?
| @touch $@ | ||
|
|
||
| # Build the provider and all SDKs and install ready for testing | ||
| build: .make/mise_install provider build_sdks install_sdks#{{- if .Config.RegistryDocs }}# build_registry_docs#{{- end }}# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you speak to this? Does build include install or not?
| @echo " build_sdks Generate and compile all SDKs" | ||
| @echo " generate_sdks Generates all SDKs" | ||
| @echo " compile_sdks Compile all previously generated SDKs" | ||
| @echo " install_sdks Install SDKs. Installs nodejs and dotnet SDKs. Noop for go, java, and python, " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: little typo here? (, )
This change is part of our refactoring of how SDK generation works for providers. It alters the makefile targets to use the tool, and adds the new tool to mise.
This change is ready to be merged and rolled out to the bridged providers
As part of this rollout we are also going to migrate overlays into a new package structure. This has been done for AWS, and I have identified the other 3 packages that need that change (pulumi-gcp, pulumi-openstack, and pulumi-azure). In addition each bridged provider will need another commit to remove the 4 SDKs that no longer need to be included in source