Hooks for Data Center

[dagguh]

It's a preview of how to inject hooks to an entire cluster. It includes wiring up the database via hooks. The old MySqlDatabase silently relied on a distant StandaloneNodeFormula to:

• install the JDBC connector
• edit a preexisting dbconfig.xml

Now, as a hook, it can do all of that on its own. Moreover, other databases can be hooked in too.

It's a draft, because it depends on #102 and it doesn't have a DC test yet. The DC test will very likely introduce a DataCenterFormula, like from aws-infrastructure, but generalized. Perhaps I'll be able to split the database and DC features into separate PRs.

[dagguh]

Currently Jira starts and MySQL starts, but Jira doesn't have connectivity to MySQL. It's because it doesn't know its IP. The ssh-ubuntu Docker code is based on testcontainers, which don't offer peer container IPs. The getTestHostIpAddress throws explicitly. I'll try to bump testcontainers or harvest the lower-level Docker code from virtual-users.

[dagguh]

To make it work I'll have to distinguish public and private IPs. A Docker network is equivalent to an AWS VPC, so the TcpHost API changes make general sense. However, notion of private/public ports doesn't generalize, such mapping is only relevant in Dockers, so that should go away from the API. The FromPort and ToPort in AWS::EC2::SecurityGroup model a range of ports, not a mapping.

[pczuj]

If there is a reason why it's not

Suggested change

	"org.seleniumhq.selenium:$module:[3.11.0, 3.999.999]"
	"org.seleniumhq.selenium:$module:[3.11.0, 4.0.0)"

then IMHO it should be noted in a comment

[dagguh]

Good catch. They polluted their release channel with unstable versions like 4.0.0-alpha-1

• put 3.999.999 rationale in the comment

[pczuj]

I don't understand this method. Why shouldn't it be inlined?

[dagguh]

Yep, inlining is ok

• inline serveTest

[dagguh]

Today I fixed docker-in-docker both in WSL and in GitHub Actions. I fixed assertions. The only red test now is RestUpgrade timeout flake on GHA.
TODO:

• add a Report that would print the appropriate log
• expose the downloaded Reports as GHA artifacts

[pczuj]

What do you think about a pattern of naming things more as what they represent in code rather than what they symolise? Naming is obviously very important because it's highly connected to how we model the system.

Maybe this class should be named DatabaseIpConfigurator instead? IMHO it doesn't represent the config, because if it would then I'd expect it to return some data. Configurator is someone/something that doesn't tell us what the config is, but it's capable of configuring which this class seem to be doing. It's like an expert who is not telling us how did he do something, yet there could be a manual that tells us how thing should be done.

[dagguh]

It does represent the config, it has the data:

 private val databaseIp: String

It could hold more data when such need is discovered.
It's also a post-install hook, which applies the data.
It does not represent the config file.

[pczuj]

It consumes the data and doesn't expose it.

Don't you think that if the data would need to be exposed it should be made as part of different class with different responsibility? I see configurator and configuration as 2 different things that should be separated. I may want to reuse configuration, yet do the process of configuration (configurator implementation) differently in some cases - in such case I'd like to reimplement configurator and not the configuration.

[dagguh]

Hmm, so lets imagine if there's a new way of applying a database IP: via YAML. Then we could add a new YamlDatabaseIpConfig alternative. It's a naming/semantics conflict, DatabaseIpConfig is no longer the canonical DTO. Or we could modify it to choose between XML and YAML modification transparently. But such impl might be impossible, so separating the behavior from the data gives us more options.

I kinda see the point.

I hoped to hide details from the caller. Now we'd have to call it DatabaseXmlConfigIpSedReplacer "just in case" someone produces an alternative impl, differing in any aspect. That's a yikes name, too enterprisey. I have this tendency to try to speak the language of the user. What does the user need? Maybe we could defer the "just in case" refactoring and pay the price when it actually happens? 😁

We gotta choose the lesser evil: user-unfriendly names or a potential deprecation cycle.

[pczuj]

Naming seems to be always hard especially when balancing between correctness and readability.

Yes this class should be called DatabaseXmlConfigIpSedReplacer, however this shows how it has too many responsibilities:

• It knows where the db config is
• It knows that the db config is in XML format and that it has single <url> node inside
• It knows the XML format
• It knows the <url> format
• It knows how to use sed (it even assumes it's available)

I believe the whole functionality was already somewhere in the JPT code (didn't I actually write it?), so I would be fine with leaving it in this granularity (and sed usage hack). Skipping some details and naming it DatabaseIpReplacer IMHO is good enough. With such name we could allow to override DbConfigXmlLocator, XmlValueReplacer, UrlParser, etc. in later changes.

[dagguh]

Indeed. The code already existed. The PR extracted these operations into a hookable API. It's the first step of decomposing DataCenterFormula and friends. Next waves of decomposition can follow afterwards.