🚀 Add Workspace Variable Set Support #497

sheneska · 2024-10-15T20:58:17Z

Description

This PR extends the Workspace Controller by adding support for variable sets. A new field spec.variablesets.[id | name] allows assigning a variable set to a workspace.
Related to #207.

How does this work?

This functionality allows users to apply existing variable sets to existing workspaces. If the variable set has been set to global, we can assume it is applied to all workspaces. The scope cannot be changed in this functionality.

If the variable set is global, we can assume it is already applied to the workspace.
If the variable set is not global, then it can be applied to a specific workspace.

Usage Example

References

Community Note

Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.
Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request.
If you are interested in working on this issue or have submitted a pull request, please leave a comment.

arybolovlev

I have left a few comments, mostly cosmetic changes to align with the patterns that we have in place.

It looks like you are on the right track with this addition. 👍🏻

I would suggest adding a usage(bare-minimum) example of a YAML manifest to the PR description. It will help you and everyone else who reviews this PR to understand how it should look from the end user's perspective.

Also, consider doing changes one by one when you work on the controller logic. For instance, once you prototyped CR and are done with API, add logic that will allow you to refer to a variable set by ID only. Make sure you cover all scenarios here, such as but not limited to: creating a new CR with VS, adding VS to an existing CR, deleting VS from CR, updating VS reference or adding a new one, etc. One this all done and works, it would be easier for you to add VS reference by name.

One more thing, please take a look at how files are organized and either move your logic to a new file now or work on it within workspace_controller.go and move it later.

All in all, good! :)

api/v1alpha2/workspace_types.go

api/v1alpha2/variablesets_validation_test.go

controllers/workspace_controller.go

arybolovlev · 2024-11-13T19:19:04Z

@sheneska, as we discussed, please make sure you have updated the FAQ to explain variable sets support logic. It is a bit tricky addition and might cause some unexpected results to the one who is not aware of the implementation details. But this is something you can add at the end.

arybolovlev

Thank you for addressing my previous comments. 👍🏻 I have left a few more.

charts/hcp-terraform-operator/crds/app.terraform.io_workspaces.yaml

controllers/workspace_controller.go

docs/examples/workspace-variable-sets.yaml

api/v1alpha2/workspace_validation.go

api/v1alpha2/workspace_validation_test.go

arybolovlev · 2024-11-25T13:07:59Z

controllers/workspace_controller_variable_sets_test.go

+		workspace = fmt.Sprintf("kubernetes-operator-%v", randomNumber())
+		variableSetName = fmt.Sprintf("variable-set-%v", randomNumber())
+		variableSetName2 = fmt.Sprintf("%v-2", variableSetName)
+		variableSetID = createTestVariableSet(variableSetName)


Please validate this function. This set of tests fails due to an error returned by HCP Terraform.

This is still an issue with the latest commit.

arybolovlev · 2024-11-25T13:14:43Z

controllers/workspace_controller_variable_sets_test.go

+	appv1alpha2 "github.com/hashicorp/hcp-terraform-operator/api/v1alpha2"
+)
+
+var _ = Describe("Workspace controller", Ordered, func() {


All in all, it's great that you've added tests here. However, they are failing due to the lack of connection between workspace reconciliation and the new logic you're working on. Please refer to my comment in the controllers/workspace_controller_variable_sets.go file.

You can run tests locally and focus only on a specific one by passing an addition argument:

$ make test TESTARGS='-ginkgo.focus "VariableSet"'

You need to export the following environment variables to make them run:

$ export TFC_TOKEN=<TFC_API_TOKEN> $ export TFC_ORG=<TFC_ORG_NAME>

This is still an issue with the latest commit.

controllers/workspace_controller_variable_sets.go

arybolovlev

It looks better now. 👍🏻 I have left a few comments regarding the reconciliation logic. Please, ensure you have fixed tests before marking the comment as resolved.

arybolovlev · 2024-11-26T08:09:22Z

controllers/workspace_controller_variable_sets_test.go

+	appv1alpha2 "github.com/hashicorp/hcp-terraform-operator/api/v1alpha2"
+)
+
+var _ = Describe("Workspace controller", Ordered, func() {


This is still an issue with the latest commit.

arybolovlev · 2024-11-26T08:09:32Z

controllers/workspace_controller_variable_sets_test.go

+		workspace = fmt.Sprintf("kubernetes-operator-%v", randomNumber())
+		variableSetName = fmt.Sprintf("variable-set-%v", randomNumber())
+		variableSetName2 = fmt.Sprintf("%v-2", variableSetName)
+		variableSetID = createTestVariableSet(variableSetName)


This is still an issue with the latest commit.

controllers/workspace_controller_variable_sets.go

arybolovlev · 2024-11-26T08:34:19Z

controllers/workspace_controller_variable_sets.go

+
+}
+
+func (r *WorkspaceReconciler) reconcileVariableSets(ctx context.Context, w *workspaceInstance, workspace *tfc.Workspace) error {


Please update the logic in this method:

It only reconcile the first variable set in the list. See my comment below.

It does API calls before they are needed.

When everything is synchronized, and during every sync period, when the operator runs a reconciliation loop, we should make as few API calls as possible. The ideal case is either 0 or 1 API call. 0 is when spec.variableSets is empty(this one you have handled), and 1 is when actual and desired states match. With the current logic, you do 2 or 3 API calls(depending on how a variable set is referred to, by name or ID).

sheneska

Did a self review - need to optimize code

sheneska · 2024-11-27T15:39:30Z

controllers/workspace_controller_variable_sets.go

+	}
+
+	// List all variable sets once, a single API call
+	listOpts := &tfc.VariableSetListOptions{


Page size = 100, what happens if there are >100?
for loop to iterate thru all pages to ensure we access everything...

sheneska · 2024-11-27T15:45:18Z

controllers/workspace_controller_variable_sets.go

+
+	for _, variableSetID := range variableSetIDs {
+		// Check if the variable set exists in the actual state
+		variableSet, found := variableSetsByID[variableSetID]


what is variableSet & variableSetsByID ?

sheneska · 2024-11-27T16:02:16Z

controllers/workspace_controller_variable_sets.go

+			}
+
+			// Searching by name instead
+			for _, vs := range variableSetList.Items {


this needs to be optimized...
extra looping

sheneska · 2024-11-27T16:45:55Z

controllers/workspace_controller_variable_sets.go

+			}
+
+			// Update page number to fetch the next set of results
+			listOpts.PageNumber = variableSetList.NextPage


consider the for loop for paginating thru all var sets,
restructuring these lines (fr line 33)

sheneska · 2024-11-27T18:24:42Z

controllers/workspace_controller_variable_sets.go

+func (w *workspaceInstance) getVariableSetIDs(ctx context.Context) ([]string, error) {
+	variableSets := w.instance.Spec.VariableSets
+
+	if variableSets == nil {


This will always error, users also have the ability to not add variable sets
consider this....

sheneska · 2024-11-27T18:28:13Z

controllers/workspace_controller_variable_sets.go

+		}
+	}
+
+	if len(variableSetIDs) == 0 {


Error message needs to be more explicit
1st check if varset was found or not...move it up

Add Workspace VarSet struct & update CRD + API reference

17618bc

sheneska requested a review from a team as a code owner October 15, 2024 20:58

sheneska marked this pull request as draft October 15, 2024 20:58

github-actions bot added documentation Improvements or additions to documentation api golang size/XL labels Oct 15, 2024

arybolovlev added this to the v2.8.0 milestone Oct 24, 2024

github-actions bot added size/XS and removed size/XL labels Oct 29, 2024

Update CRD and API reference

dfa1ba6

sheneska force-pushed the add-support-workspace-varset branch from 901e005 to dfa1ba6 Compare October 29, 2024 15:57

Edit CRD + Add validation

95fbb15

github-actions bot added crd helm-chart size/L and removed size/XS labels Nov 4, 2024

Add Validation Tests

23dabad

github-actions bot added the test label Nov 4, 2024

sheneska marked this pull request as ready for review November 4, 2024 21:29

Remove Scope

da635d0

github-actions bot added the controller label Nov 6, 2024

sheneska added 2 commits November 6, 2024 17:47

Add controller logic

b54dd12

WIP

e8b33ea

arybolovlev reviewed Nov 13, 2024

View reviewed changes

arybolovlev marked this pull request as draft November 13, 2024 19:16

WIP

9c30b95

github-actions bot added size/XL and removed size/L labels Nov 20, 2024

arybolovlev reviewed Nov 25, 2024

View reviewed changes

Add reconciler logic & cosmetic fixes

e84a0a6

arybolovlev reviewed Nov 26, 2024

View reviewed changes

Fix reconciler logic

65db417

sheneska commented Nov 27, 2024

View reviewed changes

Optimize reconciliation logic

c9d7b16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🚀 Add Workspace Variable Set Support #497

🚀 Add Workspace Variable Set Support #497

sheneska commented Oct 15, 2024 •

edited

Loading

arybolovlev left a comment

arybolovlev commented Nov 13, 2024

arybolovlev left a comment

arybolovlev Nov 25, 2024

arybolovlev Nov 26, 2024

arybolovlev Nov 25, 2024

arybolovlev Nov 26, 2024

arybolovlev left a comment

arybolovlev Nov 26, 2024

arybolovlev Nov 26, 2024

arybolovlev Nov 26, 2024

sheneska left a comment

sheneska Nov 27, 2024

sheneska Nov 27, 2024

sheneska Nov 27, 2024

sheneska Nov 27, 2024

sheneska Nov 27, 2024

sheneska Nov 27, 2024


		}

		func (r WorkspaceReconciler) reconcileVariableSets(ctx context.Context, w workspaceInstance, workspace *tfc.Workspace) error {

🚀 Add Workspace Variable Set Support #497

Are you sure you want to change the base?

🚀 Add Workspace Variable Set Support #497

Conversation

sheneska commented Oct 15, 2024 • edited Loading

Description

Usage Example

References

Community Note

arybolovlev left a comment

Choose a reason for hiding this comment

arybolovlev commented Nov 13, 2024

arybolovlev left a 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

Choose a reason for hiding this comment

arybolovlev left a 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

sheneska left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sheneska commented Oct 15, 2024 •

edited

Loading