Skip to content

Conversation

mfbx9da4
Copy link
Contributor

@mfbx9da4 mfbx9da4 commented Jun 4, 2025

No description provided.

@mfbx9da4 mfbx9da4 requested a review from a team as a code owner June 4, 2025 17:38
}()

// Mock workflow with testing enabled/disabled targets
enabledTrue := true
Copy link
Member

Choose a reason for hiding this comment

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

I really wish Go would support inline pointer value declarations or at least standard library the func Pointer[T any](v T) *T { return &v } function. 😭

for name, target := range wf.Targets {
if target.Testing != nil && target.Testing.Enabled != nil && *target.Testing.Enabled {
testedTargets = append(testedTargets, name)
break // Pick the first one with testing enabled
Copy link
Member

Choose a reason for hiding this comment

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

I'll admit I'm struggling with the best behavior choice here. I see choosing the first but that also feels like a random guess in a multi-target setup. Should we run all instead if undefined?

I think we should make a change like this either which way rather than doing nothing, but we can also improve that in the generated GHA workflow file that lands in customer repos to either require or default to better target input selection during workflow dispatch. We should be writing per-target test workflows with speakeasy configure testing so it'd be just a matter of adding Default string yaml:"default" in sdk-gen-config Target type then setting it appropriately in the CLI logic that writes out the files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I totally follow what you've said. However, there is a ticket connected to this PR. We can discuss during triage the appropriate solution and with whoever will test and finish this off.

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.

2 participants