internal/testrunner/runners/script: provide mechanism to inspect agent containers

[efd6]

Please take a look.

The addition to the get_docs test demonstrates how this can be used with a trivial example.

[efd6]

@mrodm Is this a reasonable way to reconstruct this? I did not see any direct representation of the container name for the container running the agent.

I expect that the suffix will ~always be "-1" since the run ID is a random value and we would not expect to have collisions between running agents, even in the case that a script started two or more agents (something that I would imagine would be vanishingly rare in itself).

[mrodm]

LGTM.
As a reference, this is the code where the docker project name is built:

elastic-package/internal/agentdeployer/agent.go

Line 136 in 6a690bf

composeProjectName := fmt.Sprintf("elastic-package-agent-%s-%s", d.agentName(), agentInfo.Test.RunID)

elastic-package/internal/agentdeployer/agent.go

Lines 235 to 241 in 6a690bf

	func (d *DockerComposeAgentDeployer) agentName() string {
	name := d.packageName
	if d.dataStream != "" && d.dataStream != "." {
	name = fmt.Sprintf("%s-%s", name, d.dataStream)
	}
	return name
	}

An example from nginx would be (data stream access):

elastic-package-agent-nginx-access-11932-elastic-agent-1

The only difference would be if input packages are being tested.

In that case, the docker project name would not contain the dataStream string.

elastic-package-agent-sql_input-46311-elastic-agent-1

[mrodm]

LGTM
Just a couple of comments to rename some messages to refer to the right function.

Not sure about adding the new method to the DockerComposeAgentDeployer without adding it to the AgentDeployer interface.

[mrodm]

Just re-reviewing this PR, I realised that DockerComposeAgentDeployer implements the interface AgentDeployer.

elastic-package/internal/agentdeployer/deployer.go

Line 11 in ffd949a

type AgentDeployer interface {

And this ProjectName method is not there.

I think it would be better to add this method to the interface too.

That would enforce to add this method also to KubernetesAgentDeployer, but I'm not sure that makes sense there, because in that case the agent runs directly in k8s and not in a docker-compose scenario.

elastic-package/internal/agentdeployer/kubernetes.go

Line 109 in ffd949a

func (ksd *KubernetesAgentDeployer) SetUp(ctx context.Context, agentInfo AgentInfo) (DeployedAgent, error) {

@efd6 @elastic/ecosystem WDYT about this ?

[mrodm]

I think it would be better to add this method to the interface too.

I guess it is not an issue to keep that method as a public one but not being part of the AgentDeployer interface. Specially, because this new method does not make sense in KubernetesAgentDeployer as mentioned in:

That would enforce to add this method also to KubernetesAgentDeployer, but I'm not sure that makes sense there, because in that case the agent runs directly in k8s and not in a docker-compose scenario.

[mrodm]

I think it would be better to add this method to the interface too.

I guess it is not an issue to keep that method as a public one but not being part of the AgentDeployer interface. Specially, because this new method does not make sense in KubernetesAgentDeployer as mentioned in:

That would enforce to add this method also to KubernetesAgentDeployer, but I'm not sure that makes sense there, because in that case the agent runs directly in k8s and not in a docker-compose scenario.

[efd6]

Not sure about adding the new method to the DockerComposeAgentDeployer without adding it to the AgentDeployer interface.

This was intentional and follows the Go approach to progressive code repair. The interface advertises the generic consumer's interface, but we are not the generic consumer. It's not needed right now (and likely never will be needed), so it should not be added until it is. This is for three reasons: 1. it's a breaking change, 2. a dummy method would have to be added to all the other implementers of the interface (related to 1.) and 3. "The bigger the interface, the weaker the abstraction."). This is also discussed in this "Hacking with Andrew and Brad" episode (1h, and I don't recall the exact time stamp, sorry); the gist of the discussion then is that Go is not Java and so it's not necessary to define interface inclusion on types before-hand.

I believe it would be a mistake and an antipattern to add the method to the interface at this stage.

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: 46595e0

History

• 💚 Build #6711 succeeded cecdc18
• 💔 Build #6710 failed c7f5df1
• 💔 Build #6709 failed 75b1f2c

cc @efd6

[mrodm]

Not sure about adding the new method to the DockerComposeAgentDeployer without adding it to the AgentDeployer interface.

This was intentional and follows the Go approach to progressive code repair. The interface advertises the generic consumer's interface, but we are not the generic consumer. It's not needed right now (and likely never will be needed), so it should not be added until it is. This is for three reasons: 1. it's a breaking change, 2. a dummy method would have to be added to all the other implementers of the interface (related to 1.) and 3. "The bigger the interface, the weaker the abstraction."). This is also discussed in this "Hacking with Andrew and Brad" episode (1h, and I don't recall the exact time stamp, sorry); the gist of the discussion then is that Go is not Java and so it's not necessary to define interface inclusion on types before-hand.

I believe it would be a mistake and an antipattern to add the method to the interface at this stage.

Agreed, let's keep that function there just in DockerComposeAgentDeployer.
Thanks for the references!

[teresaromero]

👍🏻