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

add support for new organization-specific workspace limit setting #425

Merged
merged 1 commit into from Jul 5, 2022

Conversation

SwiftEngineer
Copy link
Contributor

@SwiftEngineer SwiftEngineer commented Jun 10, 2022

Description

This change allows go-tfe to read/set/unset a newly created setting on the organization model. This setting is only available in TFE.

Testing plan

  1. Run local development version of TFE
  2. Run the newly updated tests in admin_organization_integration_test.go

Output from tests

It's worth noting that I had to make a few changes to the existing test (specifically, assertions about the GlobalModuleSharing flag) in order to get these tests to pass. Since those tests don't get run in CI, I'm thinking they probably haven't run in a long while.

Looking at #278, I can't figure out how the assertions could have passed, since globalModuleSharing was changed from a bool to *bool, shouldn't the assertions reflect that? Maybe the test library changed? I have no idea! 😅 This is my first PR for Golang, so if anyone could correct me here, I can happily revert my changes there and fix whatever is going on.

go tool test2json -t tmp/path-from/GoLand/___TestAdminOrganizations_Update_fetches_and_updates_organization_in_github_com_hashicorp_go_tfe.test -test.v -test.paniconexit0 -test.run ^\QTestAdminOrganizations_Update\E$/^\Qfetches_and_updates_organization\E$
=== RUN   TestAdminOrganizations_Update
--- PASS: TestAdminOrganizations_Update (3.37s)
=== RUN   TestAdminOrganizations_Update/fetches_and_updates_organization
    --- PASS: TestAdminOrganizations_Update/fetches_and_updates_organization (2.45s)
PASS

@SwiftEngineer SwiftEngineer changed the title add support for setting and unsetting organization-specific workspace… add support for new organization-specific workspace limit setting Jun 10, 2022
nullable.go Outdated
func (i NullableInt) MarshalJSON() ([]byte, error) {
return Nullable{i.value}.MarshalJSON()
}

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 added a string version of nullable as well (even though it isn't in use right now) so that folks would have an easier time understanding how they might extend Nullable to work with their own types.

JarrettSpiker
JarrettSpiker previously approved these changes Jun 10, 2022
Copy link
Contributor

@JarrettSpiker JarrettSpiker left a comment

Choose a reason for hiding this comment

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

LGTM.
Im assuming that 0 is handles separately from null on the backend, which is why we want the value to be either?

My only nitpick with that it is nil and not null in go. I guess it shows up as null in the JSON though, so 🤷

@SwiftEngineer
Copy link
Contributor Author

LGTM. Im assuming that 0 is handles separately from null on the backend, which is why we want the value to be either?

Can confirm, that's a valid and true assumption 👍

My only nitpick with that it is nil and not null in go. I guess it shows up as null in the JSON though, so 🤷

Tell me about it. I did write a test to verify that at least!

@@ -12,7 +12,7 @@ Your registry module repository will need to be a [valid module](https://www.ter
It will need the following:
1. To be named `terraform-<PROVIDER>-<NAME>`
1. At least one valid SemVer tag in the format `x.y.z`
[terraform-random-module](ttps://github.com/caseylang/terraform-random-module) is a good example repo.
[terraform-random-module](https://github.com/caseylang/terraform-random-module) is a good example repo.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

small little typo fix that I didn't think was worth a second PR, so I snuck it in here 🥷

nullable.go Outdated Show resolved Hide resolved
@barrettclark
Copy link
Contributor

@SwiftEngineer Would you mind squashing the commits once you're ready for this to be merged in?

@SwiftEngineer
Copy link
Contributor Author

@SwiftEngineer Would you mind squashing the commits once you're ready for this to be merged in?

No sweat! will do

@SwiftEngineer SwiftEngineer force-pushed the SwiftEngineer/add-organization-workspace-limit branch from 959ca1c to 9d22344 Compare June 22, 2022 21:58
@SwiftEngineer SwiftEngineer self-assigned this Jun 24, 2022
brandonc
brandonc previously approved these changes Jun 24, 2022
Copy link
Collaborator

@brandonc brandonc left a comment

Choose a reason for hiding this comment

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

Thanks! Be sure to update the changelog and it looks g2g

Comment on lines -272 to +279
assert.Equal(t, adminOrg.GlobalModuleSharing, globalModuleSharing)
assert.Equal(t, &globalModuleSharing, adminOrg.GlobalModuleSharing)
Copy link
Collaborator

@brandonc brandonc Jun 24, 2022

Choose a reason for hiding this comment

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

I'm a little confused by the argument swapping here and on L241. The other assertions seem to put the parameter second and the model first. And also by the address operator. What's going on?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's worth noting that I had to make a few changes to the existing test (specifically, assertions about the GlobalModuleSharing flag) in order to get these tests to pass. Since those tests don't get run in CI, I'm thinking they probably haven't run in a long while.

OK ! Nevermind !

@annawinkler
Copy link
Contributor

Please squash the commits & then this looks great!

@SwiftEngineer SwiftEngineer force-pushed the SwiftEngineer/add-organization-workspace-limit branch from 56bab48 to 0f2a2b9 Compare June 30, 2022 23:26
@SwiftEngineer SwiftEngineer merged commit 52e03e5 into main Jul 5, 2022
@SwiftEngineer SwiftEngineer deleted the SwiftEngineer/add-organization-workspace-limit branch July 5, 2022 17:02
@github-actions
Copy link

github-actions bot commented Jul 5, 2022

Reminder to the contributor that merged this PR: if your changes have added important functionality or fixed a relevant bug, open a follow-up PR to update CHANGELOG.md with a note on your changes.

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

5 participants