support organization default execution mode #762

SwiftEngineer · 2023-09-01T20:56:09Z

Description

Adds support for organization-level default execution modes and agent pools.

Output from tests

=== RUN   TestOrganizationsUpdate/with_default_execution_mode_and_without_agent_pool
    --- PASS: TestOrganizationsUpdate/with_default_execution_mode_and_without_agent_pool (4.83s)
=== RUN   TestOrganizationsUpdate/when_only_updating_a_subset_of_fields
    --- PASS: TestOrganizationsUpdate/when_only_updating_a_subset_of_fields (3.44s)
=== RUN   TestOrganizationsUpdate/with_default_execution_mode_of_'agent'
    --- PASS: TestOrganizationsUpdate/with_default_execution_mode_of_'agent' (6.23s)
=== RUN   TestOrganizationsUpdate/with_default_execution_mode_of_'remote'
    --- PASS: TestOrganizationsUpdate/with_default_execution_mode_of_'remote' (6.68s)
=== RUN   TestWorkspacesCreate/when_no_execution_mode_is_not_specified_in_an_organization_with_local_as_default_execution_mode
--- PASS: TestWorkspacesCreate (7.93s)
    --- PASS: TestWorkspacesCreate/when_no_execution_mode_is_not_specified_in_an_organization_with_local_as_default_execution_mode (3.99s)
=== RUN   TestWorkspacesCreate
--- PASS: TestWorkspacesCreate (12.17s)
=== RUN   TestWorkspacesCreate/when_organization_has_a_default_execution_mode
    --- PASS: TestWorkspacesCreate/when_organization_has_a_default_execution_mode (8.70s)
=== RUN   TestWorkspacesCreate/when_organization_has_a_default_execution_mode/with_setting_overwrites_set_to_false,_workspace_inherits_the_default_execution_mode
        --- PASS: TestWorkspacesCreate/when_organization_has_a_default_execution_mode/with_setting_overwrites_set_to_false,_workspace_inherits_the_default_execution_mode (1.02s)
=== RUN   TestWorkspacesCreate/when_organization_has_a_default_execution_mode/with_setting_overwrites_set_to_true,_workspace_ignores_the_default_execution_mode
        --- PASS: TestWorkspacesCreate/when_organization_has_a_default_execution_mode/with_setting_overwrites_set_to_true,_workspace_ignores_the_default_execution_mode (0.92s)
=== RUN   TestWorkspacesCreate/when_organization_has_a_default_execution_mode/when_explicitly_setting_execution_mode,_workspace_ignores_the_default_execution_mode
        --- PASS: TestWorkspacesCreate/when_organization_has_a_default_execution_mode/when_explicitly_setting_execution_mode,_workspace_ignores_the_default_execution_mode (0.92s)
=== RUN   TestWorkspacesRead
--- PASS: TestWorkspacesRead (14.50s)
=== RUN   TestWorkspacesRead/when_workspace_is_inheriting_the_default_execution_mode
    --- PASS: TestWorkspacesRead/when_workspace_is_inheriting_the_default_execution_mode (8.91s)
=== RUN   TestWorkspacesRead/when_workspace_is_inheriting_the_default_execution_mode/and_workspace_execution_mode_is_default
        --- PASS: TestWorkspacesRead/when_workspace_is_inheriting_the_default_execution_mode/and_workspace_execution_mode_is_default (0.83s)
=== RUN   TestWorkspacesUpdateWithDefaultExecutionMode
--- PASS: TestWorkspacesUpdateWithDefaultExecutionMode (10.60s)
=== RUN   TestWorkspacesUpdateWithDefaultExecutionMode/when_explicitly_setting_execution_mode,_workspace_ignores_the_default_execution_mode
    --- PASS: TestWorkspacesUpdateWithDefaultExecutionMode/when_explicitly_setting_execution_mode,_workspace_ignores_the_default_execution_mode (0.79s)
=== RUN   TestWorkspacesUpdateWithDefaultExecutionMode/with_setting_overwrites_set_to_true,_workspace_ignores_the_default_execution_mode
    --- PASS: TestWorkspacesUpdateWithDefaultExecutionMode/with_setting_overwrites_set_to_true,_workspace_ignores_the_default_execution_mode (0.83s)
=== RUN   TestWorkspacesUpdateWithDefaultExecutionMode/with_setting_overwrites_set_to_false,_workspace_inherits_the_default_execution_mode
    --- PASS: TestWorkspacesUpdateWithDefaultExecutionMode/with_setting_overwrites_set_to_false,_workspace_inherits_the_default_execution_mode (0.91s)
PASS

organization.go

JarrettSpiker · 2023-09-07T14:22:05Z

workspace.go

+	// organization or project).
+	//
+	// For example: When setting the execution-mode of a workspace to  "default", the
+	// `setting-overwrites.execution-mode` field should be set to true, because the execution-mode


This is backwards. If the value is true, that means that the workspace is overwriting the value.

workspace.go

emlanctot

looks great 💯

Jarrett's on vacaaaaaaaay 🎉 but if the comments they mentioned truly need addressing I'll happily create a follow-up PR

Uk1288 · 2023-10-13T16:46:44Z

helper_test.go

+
+	return org, func() {
+		// unset execution mode of organization
+		_, err := client.Organizations.Update(ctx, org.Name, OrganizationUpdateOptions{


For the cleanup task, instead of:

making an org update call to unset the agent

delete the agent

delete the org
can this be done in 2 calls:

1 delete org first to unset the agent since org will deleted any way
2 delete the agent

I was a little scared to do this, specifically because I know relying on atlas' cleanup logic has caused test flakes in the past, but you're probably right that I was being overprotective in this case. Thanks for the catch!

Okay, so as it turns out, we've got a bug in the current version of TFC that prevents us from being able to remove this hook. We just opened a PR to fix it, so in the meantime I added back the request to clear the default agent pool out.

edit: PR has been merged so I'm backing out the temporary fix and crossing my fingers that the nightly update fixes the problem 🤞

Uk1288 · 2023-10-13T17:12:50Z

organization_integration_test.go

@@ -263,6 +281,20 @@ func TestOrganizationsUpdate(t *testing.T) {
 		assert.EqualError(t, err, ErrInvalidOrg.Error())
 	})

+	t.Run("with default execution mode and without agent pool", func(t *testing.T) {


test case summary could be clearer, it seems to be doing something in the line of

Suggested change

t.Run("with default execution mode and without agent pool", func(t *testing.T) {

t.Run("when specifying remote execution mode with default agent pool", func(t *testing.T) {

Good catch!

Uk1288 · 2023-10-13T17:15:02Z

organization_integration_test.go

+		t.Cleanup(orgAgentTestCleanup)
+	})
+
+	t.Run("with default execution mode of 'remote'", func(t *testing.T) {


this 2 test cases seem to be doing the same thing, i.e test case
t.Run("with default execution mode of 'agent'", func(t *testing.T) {...
and
t.Run("with default execution mode of 'remote'", func(t....

Should they be merged as one?

Uk1288 · 2023-10-13T17:24:42Z

workspace_integration_test.go

@@ -621,6 +621,32 @@ func TestWorkspacesCreate(t *testing.T) {
 		assert.Equal(t, err, ErrRequiredAgentPoolID)
 	})

+	t.Run("when no execution mode is not specified in an organization with local as default execution mode", func(t *testing.T) {


the double negation made the test summary unclear.

TODO to update test summary

workspace.go

…xecution-mode

nfagerlund

Ok, this is in fairly solid shape! I have one code change to request (value version of overwrites doesn't need pointers), and several more documentation requests on account of how challenging this new pattern is going to be to our users.

I ran the new code interactively in my test project, and it works as advertised!

nfagerlund · 2023-11-01T00:13:30Z

workspace.go

+	ExecutionMode *bool `jsonapi:"attr,execution-mode,omitempty"`
+	AgentPool     *bool `jsonapi:"attr,agent-pool,omitempty"`


Right, so:

WorkspaceSettingOverwrites represents the values in a Workspace struct that we received from the API.

WorkspaceSettingOverwritesOptions represents the values we may or may not be sending to the API in a create or update options payload.

Thus, this struct (the value one) should use plain bools, and should not be using omitempty.

...I think! The theory here is that when you get a value back from the API, it should ALWAYS have either a true or false value for each of these, so we reflect that in the type system by not allowing a nil pointer as a third option. But, push back and correct me if I'm wrong in that — for example, I haven't yet tried running against an old TFE that doesn't provide this field (or doesn't provide a specific field in it, which is something we'll run into as the struct grows).

For prior art, see the nested VCSRepo / VCSRepoOptions structs, which are also doing this exact thing.

~~You sure about this? VCSRepoOptions appears to use omitempty as well from what I'm seeing.~~ I see now, you meant VCSRepo doesn't use pointers 👍

The reason I KNOW we truly need to omit these fields as empty is that , otherwise, the way the json-api serializer works is it will send nil values to TFE (along with the keys). I assume that's because the omitempty qualifier on the top level setting-ovewrites property sees that the struct beneath it is actually a hash of two false (but really nil) values? Go is funny language 😂 Anyway, old versions of TFE handle this beautifully by ignoring the field altogether, BUT newer versions of TFE interpret this to mean we want to set the setting-overwrites to false.

I believe you're 100% right that we wouldn't have problems when working with objects returned by newer versions of the API because they'll always have the fields populated. However, patch requests where we only modify the (for example) description of the workspace, will cause these empty keys to be sent to the API. I'm making the assumption that VCSRepoOptions is set up the exact same way for that reason... but I could be wrong!

For the lurkers out there: Nick and I chatted about this in the #team-tf-cli slack channel. The end result is we'd like to use pointers SOLELY in the interest of making supporting backwards-compatibility easier. By allowing these fields to be pointers, users can check to see if the attributes are nil or false, which can give go-tfe insight into what version of TFE it is interacting with.

workspace.go

nfagerlund · 2023-11-01T00:43:30Z

workspace.go

+	// Struct containing boolean values that indicate whether the resolved value of
+	// a setting should be decided by something other than the workspace (i.e. a related
+	// organization or project).
+	//
+	// For example: When setting the execution-mode of a workspace to "default", the
+	// `setting-overwrites.execution-mode` field should be set to false, because the execution-mode
+	// will be overwritten by either the organization or project.


Same suggestion for this instance as for the occurrence above.

nfagerlund · 2023-11-01T00:49:40Z

workspace.go

+	ExecutionMode *bool `json:"execution-mode,omitempty"`
+	AgentPool     *bool `json:"agent-pool,omitempty"`


This whole scheme is extremely indirect, so let's start things off right by establishing some conventions! In the create/update options version of these overwrites struct, let's document each field with the exact identity of the parent setting that gets used if the workspace doesn't set its own value. (We can be lazy in the value version of the struct, because the whole point of the scheme is so that people can just usually just read the top-level settings values and go home instead of doing inheritance math.)

Suggested change

ExecutionMode *bool `json:"execution-mode,omitempty"`

AgentPool *bool `json:"agent-pool,omitempty"`

// If false, the workspace will defer to its organization's DefaultExecutionMode value.

ExecutionMode *bool `json:"execution-mode,omitempty"`

// If false, the workspace will defer to its organization's DefaultAgentPool value.

AgentPool *bool `json:"agent-pool,omitempty"`

(Extra blank line at the top is so nothing interprets your comment about jsonapi field tags as being part of the documentation for the first field.)

Love this! Only caveat I might add is that currently we are working on allowing the project to overwrite this value too 😓 So I'll just slightly modify what you've suggested here!

nfagerlund · 2023-11-01T00:52:40Z

CHANGELOG.md

+<!-- Please also include if this is a Bug Fix, Enhancement, or Feature -->
+
+## Features
+* Added support for assigning agent pools at the organization level via a `default` agent pool by @SwiftEngineer [#762](https://github.com/hashicorp/go-tfe/pull/762)


This is a little incomplete! It should probably be multiple bullet points.

Org-level default agent pool

Org-level default execution mode

New WorkspaceSettingOverwritesOptions field for allowing workspaces to defer some settings to a default from their org or project.

Also, careful of the backticks — as written, that seems to indicate there's a new agent pool value of default available, which isn't quite right.

Good callout on adding WorkspaceSettingOverwritesOptions on there and the extra backticks!

I made one small change in that I merged the top two suggestions ("Org-level default agent pool" and "Org-level default execution mode") into one. That's because I don't want to give users the impression that they can set a default agent pool WITHOUT setting a default execution mode. In other words, the default agent pool depends on the default execution mode being set to agent.

Co-authored-by: Nick Fagerlund <nick.fagerlund@gmail.com>

workspace.go

Co-authored-by: Nick Fagerlund <nick.fagerlund@gmail.com>

nfagerlund

🚀

github-actions · 2023-11-06T15:51:19Z

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.

JarrettSpiker previously requested changes Sep 7, 2023

View reviewed changes

SwiftEngineer marked this pull request as ready for review September 26, 2023 16:40

SwiftEngineer requested a review from a team as a code owner September 26, 2023 16:40

SwiftEngineer requested a review from JarrettSpiker September 26, 2023 16:41

emlanctot previously approved these changes Sep 28, 2023

View reviewed changes

SwiftEngineer dismissed emlanctot’s stale review via 4c3a76c September 29, 2023 17:32

SwiftEngineer force-pushed the SwiftEngineer/support-organization-default-execution-mode branch 2 times, most recently from 28d7d0b to 0ef3e90 Compare October 10, 2023 22:13

Uk1288 reviewed Oct 13, 2023

View reviewed changes

SwiftEngineer requested a review from Uk1288 October 26, 2023 19:17

add support for org-level default agent pools and execution mode

a12cd99

SwiftEngineer force-pushed the SwiftEngineer/support-organization-default-execution-mode branch from 9646442 to a12cd99 Compare October 26, 2023 23:22

SwiftEngineer added 2 commits October 26, 2023 16:22

Merge branch 'main' into SwiftEngineer/support-organization-default-e…

36f3328

…xecution-mode

Merge branch 'main' into SwiftEngineer/support-organization-default-e…

5931a32

…xecution-mode

nfagerlund requested changes Nov 1, 2023

View reviewed changes

SwiftEngineer and others added 3 commits November 1, 2023 16:13

Apply suggestions from code review

e853db4

Co-authored-by: Nick Fagerlund <nick.fagerlund@gmail.com>

clarify setting overwrites in comments

a1a8418

update changelog

3c85b84

SwiftEngineer requested a review from nfagerlund November 1, 2023 23:23

SwiftEngineer added 2 commits November 1, 2023 16:36

fmt

fcbdb71

document setting overwrite fields

0b4d42a

nfagerlund reviewed Nov 2, 2023

View reviewed changes

workspace.go Show resolved Hide resolved

Update workspace.go

db156fd

Co-authored-by: Nick Fagerlund <nick.fagerlund@gmail.com>

nfagerlund approved these changes Nov 2, 2023

View reviewed changes

SwiftEngineer merged commit f4b7207 into main Nov 6, 2023
10 checks passed

SwiftEngineer deleted the SwiftEngineer/support-organization-default-execution-mode branch November 6, 2023 15:51

SwiftEngineer mentioned this pull request Nov 21, 2023

support for organization-level default execution mode and agent pool hashicorp/terraform-provider-tfe#1137

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support organization default execution mode #762

support organization default execution mode #762

SwiftEngineer commented Sep 1, 2023 •

edited

JarrettSpiker Sep 7, 2023

emlanctot left a comment •

edited

Uk1288 Oct 13, 2023 •

edited

SwiftEngineer Oct 23, 2023

SwiftEngineer Oct 26, 2023 •

edited

Uk1288 Oct 13, 2023

SwiftEngineer Oct 23, 2023

Uk1288 Oct 13, 2023

Uk1288 Oct 13, 2023 •

edited by SwiftEngineer

nfagerlund left a comment •

edited

nfagerlund Nov 1, 2023

SwiftEngineer Nov 1, 2023 •

edited

SwiftEngineer Nov 2, 2023

nfagerlund Nov 1, 2023

nfagerlund Nov 1, 2023

SwiftEngineer Nov 1, 2023

nfagerlund Nov 1, 2023 •

edited

SwiftEngineer Nov 1, 2023

nfagerlund left a comment

github-actions bot commented Nov 6, 2023

	t.Run("with default execution mode and without agent pool", func(t *testing.T) {
	t.Run("when specifying remote execution mode with default agent pool", func(t *testing.T) {

		ExecutionMode *bool `jsonapi:"attr,execution-mode,omitempty"`
		AgentPool *bool `jsonapi:"attr,agent-pool,omitempty"`

		ExecutionMode *bool `json:"execution-mode,omitempty"`
		AgentPool *bool `json:"agent-pool,omitempty"`

support organization default execution mode #762

support organization default execution mode #762

Conversation

SwiftEngineer commented Sep 1, 2023 • edited

Description

Output from tests

Choose a reason for hiding this comment

emlanctot left a comment • edited

Choose a reason for hiding this comment

Uk1288 Oct 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SwiftEngineer Oct 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uk1288 Oct 13, 2023 • edited by SwiftEngineer

Choose a reason for hiding this comment

nfagerlund left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SwiftEngineer Nov 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfagerlund Nov 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfagerlund left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 6, 2023

SwiftEngineer commented Sep 1, 2023 •

edited

emlanctot left a comment •

edited

Uk1288 Oct 13, 2023 •

edited

SwiftEngineer Oct 26, 2023 •

edited

Uk1288 Oct 13, 2023 •

edited by SwiftEngineer

nfagerlund left a comment •

edited

SwiftEngineer Nov 1, 2023 •

edited

nfagerlund Nov 1, 2023 •

edited