Accept CLI option for the number of parallel ops in a test run's plan/apply#36323
Accept CLI option for the number of parallel ops in a test run's plan/apply#36323
Conversation
liamcervante
left a comment
liamcervante
left a comment
There was a problem hiding this comment.
This looks good to me - I'd just hold off on merging until we're sure we can everything else done in time for the v1.11 release.
dsa0x
force-pushed
the
sams/tf-test-op-parallel
branch
2 times, most recently
from
b60a2d7 to
e4f9f4c
Compare
| cmdFlags.BoolVar(&jsonOutput, "json", false, "json") | ||
| cmdFlags.StringVar(&test.JUnitXMLFile, "junit-xml", "", "junit-xml") | ||
| cmdFlags.BoolVar(&test.Verbose, "verbose", false, "verbose") | ||
| cmdFlags.IntVar(&test.OperationParallelism, "parallelism", DefaultParallelism, "parallelism") |
There was a problem hiding this comment.
I'm thinking of the future, if we ever want the users to control parallelism of the test runs themselves. Perhaps this should actually be operation-parallelism instead of just parallelism. Or do you think it should be parellelism here and the other one could be test-parallelism?
There was a problem hiding this comment.
If we are going to have that as a flag, then yes operation-parallelism makes sense. However, I think we can let runs configuration be defined in the file config block that we will be introducing. If we then ever introduce file parallelism, then file-parallelism seems fine. What do you think?
There was a problem hiding this comment.
I think it's likely we'll still get a request to control the parallelism from the CLI even if we provide the option in the configuration. For example, we have #34359 asking to override the command attribute of run blocks from the CLI.
But, perhaps keeping parallelism to mean the operation parallelism across all commands (so it's the same flag for plan, apply, test, etc) is a good idea. We can then lean into file-parallelism and run-parallelism as options for that level of customisation.
| Filters: runner.Filters, | ||
| TestDirectory: tfe.String(runner.TestingDirectory), | ||
| Verbose: tfe.Bool(runner.Verbose), | ||
| Parallelism: tfe.Int(runner.OperationParallelism), |
There was a problem hiding this comment.
Just want to sanity check, that this being 0 will be overridden with a default at the API level?
There was a problem hiding this comment.
When the value is not given, i.e when the API calls the CLI, we use a default value. That override is done here, not at the API level
| cmdFlags.BoolVar(&jsonOutput, "json", false, "json") | ||
| cmdFlags.StringVar(&test.JUnitXMLFile, "junit-xml", "", "junit-xml") | ||
| cmdFlags.BoolVar(&test.Verbose, "verbose", false, "verbose") | ||
| cmdFlags.IntVar(&test.OperationParallelism, "parallelism", DefaultParallelism, "parallelism") |
There was a problem hiding this comment.
I think it's likely we'll still get a request to control the parallelism from the CLI even if we provide the option in the configuration. For example, we have #34359 asking to override the command attribute of run blocks from the CLI.
But, perhaps keeping parallelism to mean the operation parallelism across all commands (so it's the same flag for plan, apply, test, etc) is a good idea. We can then lean into file-parallelism and run-parallelism as options for that level of customisation.
dsa0x
force-pushed
the
sams/tf-test-op-parallel
branch
from
e4f9f4c to
1059cc5
Compare
liamcervante
left a comment
liamcervante
left a comment
There was a problem hiding this comment.
Can we just update the docs within this PR as well? https://github.com/hashicorp/terraform/blob/main/website/docs/cli/commands/test.mdx#general-options
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
2 participants
Fixes #34237
This PR ensures we now accept a
-parallelism=noption which sets the number of parallel operations in a test run's plan/apply operation.Target Release
1.11.x
CHANGELOG entry