-
Notifications
You must be signed in to change notification settings - Fork 129
Allow to define more than one service deployer and choose the deployer via configuration setting #2638
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
Conversation
From now on, there could be more than one service deployer defined in the `_dev/deploy` folder. This new setting helps us to choose which specific specific service deployer will be used by each test. If this new setting is not present in the configuration file, there must be zero or one service deployers defined.
|
/test |
|
test integrations |
|
Created or updated PR in integrations repository to test this version. Check elastic/integrations#14180 |
| devDeployPath, err := servicedeployer.FindDevDeployPath(servicedeployer.FactoryOptions{ | ||
| DataStreamRootPath: options.DataStreamRootPath, | ||
| DevDeployDir: options.DevDeployDir, | ||
| PackageRootPath: options.PackageRootPath, | ||
| }) |
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.
Used the same functions as in the servicedeployer module.
| // Not included in package-spec validation for deployer name | ||
| if c.Deployer != "" && !slices.Contains(allowedDeployerNames, c.Deployer) { | ||
| return nil, fmt.Errorf("invalid deployer name %q in system test configuration file %q, allowed values are: %s", c.Deployer, configFilePath, strings.Join(allowedDeployerNames, ", ")) | ||
| } |
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.
Added validation here instead of package-spec. In package-spec this file has additionalProperties: true
|
/test |
jsoriano
left a comment
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 good.
internal/agentdeployer/factory.go
Outdated
|
|
||
| func findAgentDeployers(devDeployPath string) ([]string, error) { | ||
| fis, err := os.ReadDir(devDeployPath) | ||
| func findAgentDeployers(devDeployPath, expectedDeployer string) ([]string, error) { |
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.
Nit. This method returns now a single deployer or none, right? We could change it to return a single value, and we could return a named error in case of no deployer found.
| func findAgentDeployers(devDeployPath, expectedDeployer string) ([]string, error) { | |
| func findAgentDeployer(devDeployPath, expectedDeployer string) (string, error) { |
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.
Updated method to just return one single deployer or none (empty string).
And it has been changed it to fail if deployer is set in the test configuration file and that expected agent deployer is not found in the deploy folder.
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.
Nit. Could we also have a failing case, with a package that defines multiple deployers but only defines one?
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.
Not sure what you mean for this failing case.
One data stream that in its system test configuration sets "docker" as deployer_name setting but that deployer is not present under any _dev/deploy folder?
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.
Oh sorry, I mispelled, I meant a package that defines multiple deployers, but uses none. So that this error is reproduced: expected to find only one agent deployer in...
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.
Added a test package nginx_missing_deployer with two errors:
- data_stream access requires
tfdeployer but it is not present in the_dev/deployfolder from the data stream. - data_stream error uses the
_dev/deployfolder (from package level) where there are two deployers defined, but it does not setdeployerfield in the system test configuration.
💚 Build Succeeded
History
cc @mrodm |
jsoriano
left a comment
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 good, thanks for adding the test cases!
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.
👍
Relates #1302
This PR allows us to define more than one service deployer in the
_dev/deployfolder (at package or data stream level).If there are more than one service deployer defined, it must be added to the test system configuration file a new setting
deployer. This new setting helps us to choose which specific specific service deployer will be used by each test. Example:dockerdeployer:terrafirndeployer:If this new setting is not present in the configuration file, there must be zero or one service deployers defined (same behavior as before).
Added a new test package where this new feature is used:
nginx_multiple_servicesThis test package is a subset of the
nginxtest package (just access and error data-stream and no kibana assets).Author's checklist
elastic-package: Test elastic-package#2638 - DO NOT MERGE integrations#14180nginx_multiple_services) defining both docker and terraform service deployers.