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

chore!: remove default org state #134

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

aslilac
Copy link
Member

@aslilac aslilac commented Nov 8, 2024

as an alternative to this, you should just do...

data "coderd_organization" "default" {
  is_default = true
}

...and refer to coderd_organization.default.id when needed.

What we're currently doing is not truly using the default organization, and hides details from the user that I think ultimately make the experience more dangerous and confusing.

@aslilac aslilac requested a review from ethanndickson November 8, 2024 23:17
Copy link
Member

@ethanndickson ethanndickson left a comment

Choose a reason for hiding this comment

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

This is a breaking change, and should only get added when we're ready for the 1.0 release.

Computed: true,
Optional: true,
Copy link
Member

Choose a reason for hiding this comment

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

This should be Required now, and also on all the other resources.

@ethanndickson ethanndickson changed the title chore: remove default org state DNM: chore!: remove default org state Nov 12, 2024
@aslilac
Copy link
Member Author

aslilac commented Nov 12, 2024

according to semver, every 0.0.x release is supposed to be considered "breaking".

we could even do 0.1.x, but I think this is something we should get feedback from long before 1.0, not afterwards.

@ethanndickson
Copy link
Member

ethanndickson commented Nov 13, 2024

That's fair - I just don't want to get in the habit of regularly breaking people's Terraform configs. My other hesitance is that this becomes annoying for the majority of single organization deployments. (Maybe a multi-org use-case ends up having multiple Terraform configs too, it's not unreasonable for different template admins to handle their IaC independently? Can't say for sure)

I feel like we could make the experience 'safer' without sacrificing any of the convenience of the default. What would be your thoughts on keeping the default_organization_id attribute on the provider optional, but returning a custom error if both it, and the organization id on the resource, weren't supplied? That way there'd be no implicit org selection.

@aslilac
Copy link
Member Author

aslilac commented Nov 14, 2024

  • Allowing them to set a global default adds a lot of complexity to the provider
  • It reduces our ability to validate the config during a plan (because organization_id would have to be optional on every resource that takes one)
  • It's really not that hard for them to just add...
data "coderd_organization" "default" {
  is_default = true
}

resource "..." "..." {
  organization_id = coderd_organization.default
}

It's a tiny bit of extra code for them, but avoids a lot of complexity, confusion, and magic for everyone.

@ethanndickson
Copy link
Member

I don't really agree that it adds a huge amount of complexity to the provider, user-facing or implementation wise, but I agree, it would be incredibly annoying for terraform plan to not pick up that you haven't set an org ID in either place. I've decided the implicit org selection (the first the user is a member of) is pretty bad, so yeah, let's just get rid of it for now, and then we can include it in the next release, where we're already bumping the minor version.

@ethanndickson ethanndickson changed the title DNM: chore!: remove default org state chore!: remove default org state Dec 19, 2024
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