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-1068 Migrate pipeline resource to framework #345

Merged
merged 42 commits into from Aug 14, 2023

Conversation

jradtilbrook
Copy link
Member

@jradtilbrook jradtilbrook commented Aug 2, 2023

PR checklist:

  • docs/ updated
  • tests added
  • an example added to examples/ (useful to demo new field/resource)
  • CHANGELOG.md updated with pending release information

Theres a few things I want to test manually

  • changing cluster from empty string to null
  • provider_settings is technically a list block but we shouldn't allow that. what happens if its just changed under the hood
  • ... TBD

@@ -105,7 +105,7 @@ func TestAccPipeline_add_remove_withcluster(t *testing.T) {
testAccCheckPipelineRemoteValues(&resourcePipeline, "Test Pipeline foo"),
// Confirm the pipeline has the correct values in terraform state
resource.TestCheckResourceAttr("buildkite_pipeline.foobar", "name", "Test Pipeline foo"),
resource.TestCheckResourceAttr("buildkite_pipeline.foobar", "cluster_id", ""),
resource.TestCheckNoResourceAttr("buildkite_pipeline.foobar", "cluster_id"),
Copy link
Member Author

Choose a reason for hiding this comment

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

The resource now sets this as null instead of an empty string

Copy link
Contributor

Choose a reason for hiding this comment

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

Noting testAccPipelineConfigBasicWithTeam("foo") clears the cluster ID off the pipeline - and new impl sets it as null - this LG

@jradtilbrook jradtilbrook marked this pull request as ready for review August 2, 2023 08:09
@jradtilbrook
Copy link
Member Author

Alright I think this is ready for review again. With framework, when defining a block attribute you cannot set it as required or a default value. And the framework also performs validation on the state. This causes trouble for the provider_settings block. It is optional which means that the provider cannot set any values into state if the user has not defined it. The SDKv2 version of the provider would still perform some actions using default values which is not possible now. And I cannot find a way around it.

I think long-term, moving to protocol v6 and converting it to a nested attribute is the way to go

@jradtilbrook jradtilbrook marked this pull request as ready for review August 9, 2023 07:13
@mcncl
Copy link
Contributor

mcncl commented Aug 9, 2023

@jradtilbrook I think maybe we let Pagerduty know they need to look at updating sooner rather than later?

@jradtilbrook
Copy link
Member Author

@jradtilbrook I think maybe we let Pagerduty know they need to look at updating sooner rather than later?

@mcncl yeah I think so. The good news is that the latest release has basically all the new features we were aiming for for v1. Its just some bug fixes from here. So if they can pin their version now, they won't really miss out on anything (especially if they haven't hit the cases for the bugs)

james2791
james2791 previously approved these changes Aug 11, 2023
Copy link
Contributor

@james2791 james2791 left a comment

Choose a reason for hiding this comment

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

I think this is at a point of rolling-in, LGTM 👍

buildkite/graphql/pipeline.graphql Show resolved Hide resolved
buildkite/resource_pipeline.go Outdated Show resolved Hide resolved
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.

None yet

3 participants