feat: skip destroy for schematics resources #703
Conversation
|
/run pipeline |
testschematic/tests.go
Outdated
| // create new schematic service with authenticator | ||
| var svc *SchematicsTestService | ||
|
|
||
| func (options *TestSchematicOptions) RunSchematicTest() (*SchematicsTestService, error) { |
There was a problem hiding this comment.
I'm not sure this change (creating package global for the service pointer and returning it from the Run function) is necessary. The original design was to create a new service object, only if not initially provided in options struct, and then store that pointer in the options struct to be used later if needed.
The change that is likely needed here would be to make options.schematicsTestSvc a public struct attribute by capatalizing the name: options.SchematicsTestSvc. By making that attribute public you can then reference it inside this package AND outside in the terraform unit test, and avoid having a package global containing a pointer (which is sometimes avoided due to complexity).
I would remove the global svc, remove the return value, and just change the options attribute to public (simply capatalize the attribute name).
| @@ -143,45 +146,57 @@ func (options *TestSchematicOptions) RunSchematicTest() error { | |||
| } | |||
| } | |||
|
|
|||
| // ------ DESTROY ------ | |||
| // only run destroy if we had potentially created resources | |||
| if svc.TerraformResourcesCreated { | |||
There was a problem hiding this comment.
If you are moving the resource destroy from this block, you need to still replace it with your new TestTearDown() or similar.
Keep in mind thought that in this test case there are TWO different destroy that take place:
- The destroy of the applied resources
- The delete of the workspace
The block removed here is covering item 1, the destroy of the applied resources, NOT the deletion of the workspace.
Perhaps a short meeting or slack conversation can explain this better.
testschematic/tests.go
Outdated
| _, deleteWsErr := svc.DeleteWorkspace() | ||
| if deleteWsErr != nil { | ||
| options.Testing.Logf("[SCHEMATICS] WARNING: Schematics WORKSPACE DELETE failed! Remove manually if required. Name: %s (%s)", svc.WorkspaceName, svc.WorkspaceID) | ||
| func testTearDown(options *TestSchematicOptions) { |
There was a problem hiding this comment.
The old testTearDown ONLY covered the deletion of the workspace, NOT the destroy of the applied resources.
You are replacing it and combining the destruction of applied resources AND the delete of the workspace. This may alter how this works, as you still need to call the destroy of resources in RunSchematicsTest if the skip is not set, and keep seperate the destruction of the workspace (which was done on purpose to leave workspace around on failed test due to logs only existing there).
This might have to be split into two functions. Perhaps further explanation can be done on a schematics thread or short meeting.
testschematic/test_options.go
Outdated
| @@ -189,6 +192,8 @@ func TestSchematicOptionsDefault(originalOptions *TestSchematicOptions) *TestSch | |||
| newOptions.SchematicsApiURL = DefaultSchematicsApiURL | |||
| } | |||
|
|
|||
| newOptions.SkipTestTearDown = false | |||
There was a problem hiding this comment.
should only default to false if the value is not set in originalOptions (if it is set to true).
In golang, the default for a bool is already false, so setting this here is probably optional.
|
/run pipeline |
Aashiq-J
changed the title
|
@toddgiguere @Aashiq-J Did we decide we still proceed with these changes? |
| } | ||
| } | ||
| } | ||
| options.SchematicsTestSvc = svc |
There was a problem hiding this comment.
This should be higher up in the function and not left this late. Ideally you would set the options pointer for the service right after it is initialized on line 31. If the service was already set in the options (line 33) then it does not need to be reassigned.
| // testTearDown is a helper function, typically called via golang "defer", that will clean up and remove any existing resources that were | ||
| // created for the test. | ||
| // The removal of some resources may be influenced by certain conditions or optional settings. | ||
| func testTearDown(svc *SchematicsTestService, options *TestSchematicOptions) { | ||
| func (options *TestSchematicOptions) testTearDownResources() { |
There was a problem hiding this comment.
If tear down of resources is private function (cannot be called outside of this package), then it shouldn't be a method on "options" it should be a method on SchematicsTestService.
Description
Use case:
Release required?
x.x.X)x.X.x)X.x.x)Release notes content
Run the pipeline
If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.
Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:
Checklist for reviewers
For mergers