-
Notifications
You must be signed in to change notification settings - Fork 101
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
support organization default execution mode #762
support organization default execution mode #762
Conversation
workspace.go
Outdated
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is backwards. If the value is true
, that means that the workspace is overwriting the value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great 💯
Jarrett's on vacaaaaaaaay 🎉 but if the comments they mentioned truly need addressing I'll happily create a follow-up PR
28d7d0b
to
0ef3e90
Compare
helper_test.go
Outdated
|
||
return org, func() { | ||
// unset execution mode of organization | ||
_, err := client.Organizations.Update(ctx, org.Name, OrganizationUpdateOptions{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🤞
organization_integration_test.go
Outdated
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test case summary could be clearer, it seems to be doing something in the line of
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
organization_integration_test.go
Outdated
t.Cleanup(orgAgentTestCleanup) | ||
}) | ||
|
||
t.Run("with default execution mode of 'remote'", func(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
workspace_integration_test.go
Outdated
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the double negation made the test summary unclear.
- TODO to update test summary
9646442
to
a12cd99
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
workspace.go
Outdated
ExecutionMode *bool `jsonapi:"attr,execution-mode,omitempty"` | ||
AgentPool *bool `jsonapi:"attr,agent-pool,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 bool
s, 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You sure about this? I see now, you meant VCSRepoOptions
appears to use omitempty
as well from what I'm seeing.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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same suggestion for this instance as for the occurrence above.
workspace.go
Outdated
ExecutionMode *bool `json:"execution-mode,omitempty"` | ||
AgentPool *bool `json:"agent-pool,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
CHANGELOG.md
Outdated
<!-- 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
Co-authored-by: Nick Fagerlund <nick.fagerlund@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
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. |
Description
Adds support for organization-level default execution modes and agent pools.
Output from tests