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

SUP-1681: Webhook event type definition for struct literal creation #175

Merged
merged 5 commits into from
Feb 1, 2024

Conversation

james2791
Copy link
Contributor

@james2791 james2791 commented Jan 30, 2024

Feat (Webhook event types): Webhook event type definitions for allowing creation via struct literals

PR checklist:

  • tests added -> (All pipeline template interaction with the REST API) -> Tests consistent
  • examples of each service action (List, Get etc) added to the relevant examples/ folder -> No services altered
  • CHANGELOG.md updated with pending release information

In previous version of Go-Buildkite; the types of agent, build, job webhook events were created as structs without inner field exporting; meaning that instances of these types couldn't be created in a struct literal manner. A instance of the base type was required first before adding its sub-fields:

jobFinishedEvent := buildkite.JobFinishedEvent{}
jobFinishedEvent.Pipeline = &buildkite.Pipeline{
	Name: buildkite.String("Android App"),
	... additional fields ...
}

This PR exports the inner types so that instances can be struct literal created while also keeping the above consuming model above in place - for example:

jobFinishedEvent := buildkite.JobFinishedEvent{
	JobEvent: buildkite.JobEvent{
		Pipeline: &buildkite.Pipeline{
			Name: buildkite.String("Android App"),
		},
		Build: ...
		Job: ...
	},
}

Addresses #170

@james2791 james2791 changed the title SUP-1661: Webhook event type literals SUP-1681: Webhook event type literals Jan 30, 2024
@james2791 james2791 changed the title SUP-1681: Webhook event type literals SUP-1681: Webhook event type definition for struct literal creation Jan 31, 2024
@james2791 james2791 marked this pull request as ready for review January 31, 2024 22:54
@mcncl
Copy link
Contributor

mcncl commented Jan 31, 2024

This change also means that the aliased types cannot have their own fields, they must use all and only the fields of their "event" types.

@mcncl
Copy link
Contributor

mcncl commented Feb 1, 2024

Would require a major bump due to lack of backwards compatibility.

Copy link
Contributor

@jradtilbrook jradtilbrook left a comment

Choose a reason for hiding this comment

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

What if we just exported the inner fields so they could be accessed? I don't know if keeping them private is that useful

@james2791
Copy link
Contributor Author

james2791 commented Feb 1, 2024

👆

I think thats a nice approach; similar to what I had implemented first without the inner field type declaration?

Would allow for creation like this:

jobFinishedEvent := buildkite.JobFinishedEvent{
	JobEvent: buildkite.JobEvent{
		Pipeline: &buildkite.Pipeline{
			Name: buildkite.String("Android App"),
		},
		Build: ...
		Job: ...
		...
	},
}

@jradtilbrook
Copy link
Contributor

Yeah I think that seems okay. And that should be backwards compatible as well right?

@james2791
Copy link
Contributor Author

james2791 commented Feb 1, 2024

Correct - same process in creating these struct instances as before; export just gives the chance to create them with the inner fields (i.e with declaring the JobEvent for any of the JobXEvent types - similar for agent/build ones). Thankfully its not a drastic type change that would need to constitute a x.0.0 release 👍

@james2791 james2791 merged commit 35f6a1b into main Feb 1, 2024
1 check passed
@james2791 james2791 deleted the SUP-1661-Webhook-Type-Literals branch February 1, 2024 05:56
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.

3 participants