-
Notifications
You must be signed in to change notification settings - Fork 6
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
Ability to select SDK languages #1063
Conversation
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.
Overall approach looks good. Need clarification on if the comment-dispatch change was accidental or if we need to adjust the settings name somehow.
Out of interest - why can't external providers run the auto-release hooks? Is it possible just to make this work for them too?
@@ -1,3 +1,4 @@ | |||
#{{- if .Config.enableAutoRelease -}}# |
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 a little odd - this workflow is about auto-commenting, not auto-releasing.
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.
@ringods what was your thoughts here?
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 this workflow is also pretty pulumi-specific and outside parties may want to enable and disable this, but the naming of the flag makes this confusing.
provider-ci/internal/pkg/templates/bridged-provider/.github/workflows/build_sdk.yml
Show resolved
Hide resolved
@danielrbradley AFAICS, the auto release setup depends on the Pulumi bot setup. If I want this to work for third party providers, I want to find a solution based on the standard github |
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 with the enableAutoRelease
flag it might make sense to add an external provider to the test-providers
so we can see one in action?
provider-ci/internal/pkg/templates/bridged-provider/.github/workflows/build_sdk.yml
Show resolved
Hide resolved
@@ -1,3 +1,4 @@ | |||
#{{- if .Config.enableAutoRelease -}}# |
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 this workflow is also pretty pulumi-specific and outside parties may want to enable and disable this, but the naming of the flag makes this confusing.
@@ -75,6 +75,9 @@ freeDiskSpaceBeforeTest: false | |||
# Set to true to clear disk space before running sdk build jobs. | |||
freeDiskSpaceBeforeSdkBuild: false | |||
|
|||
# Enables the automatic release of the provider | |||
enableAutoRelease: true |
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 love this flag; I think the terminology is a bit confusing. When I first read it, my thought was, "wait, what is auto release?"
I think your PR comment makes sesne as a code comment here.
enableAutoRelease (default: true): used for Pulumi, but typically not for third party providers. Changes:
- Disables the publish step of the main workflow
- Prevents generating repository dispatch workflows like command-dispatch and release-command
Further thoughts:
Could we call it "enablePublish" or even "skipPublish"? Would it make sense to have a global skipWorkflow
flag to allow for maximum configurability?
We could even have a skipPulumiInternals
switch since this is explicitly targeted at external users?
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.
Earlier today, I had a short review call for this PR with @danielrbradley. To prevent adding a lot more configuration options to the setup, an alternative setup would be to create a separate template folder aimed at thirdparty bridged providers, reshuffle some files between the different folders and update generate.go
here:
ci-mgmt/provider-ci/internal/pkg/generate.go
Lines 69 to 75 in b6d3a9f
switch templateName { | |
case "bridged-provider": | |
// Render more specific templates last to allow them to override more general templates. | |
return []string{"provider", "dev-container", "bridged-provider"}, nil | |
default: | |
return nil, fmt.Errorf("unknown template: %s", templateName) | |
} |
How does that sound to you?
649b65f
to
9f270e3
Compare
Changing `node` to `nodejs` in tool installation task This brings consistency with the name of the language used in the GHA `language` build matrix
9f270e3
to
02bf75d
Compare
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 with all these changes 👍
Follow-up to #1063 The nuget command fails if dotnet wasn't installed.
Follow-up to #1063 The nuget command fails if dotnet wasn't installed. Example failure: https://github.com/pulumi/pulumi-azure/actions/runs/10815039270/job/30003728480?pr=2387 This only affects providers which have enabled `freeDiskSpaceBeforeTest` because they will have a default version of dotnet available, and therefore also wasn't caught by the downstream tests. For the sake of consistency, we'll also only configure the pip virtualenv if we're atually running python tests.
This change pulls the latest ci-mgmt changes in order to reduce the size of provider binaries. In detail, we're now setting the `-s -w` linker flags to strip debug symbols. This cuts the provider binary size by roughly 300MB, resulting in a 32% smaller provider binary. Note: I also had to change `toolVersions.node` to `toolVersions.nodejs` because the configuration is now type checked. The `toolVersions.node` configuration option was renamed to `nodejs` in this PR: pulumi/ci-mgmt#1063
Update Sep 9: dropped the
enableAutoRelease
from this PR. Will redo this as a separate PR.Round 2 to add support for third-party providers:
organization
to 2 more places where it is neededlanguages
(default: all supported languages): third party providers can configure a subset of the supported languages if they don't have the publishing infrastructure ready for all the languages. Changes:The changes in this PR are focussed on the
main
workflow. Testing this onpulumiverse/pulumi-acme
already got me this far:https://github.com/pulumiverse/pulumi-acme/actions/runs/10596199445