eiffel-community / eiffel-gerrit-herald Goto Github PK

Description

Periodic builds are setup for this project on Nordix Infrastructure as part of Eiffel Infrastructure and CI/CD establishment work Phase 1.

It is important the builds are reviewed by the project to ensure the build process is correct.
It would also be good if the artifact is fetched from Nordix Registry and verify it works.

Here are more details.

• The job created for this project on Nordix Jenkins runs against the master branch of the project and attempts building container image using podman
• The job regularly polls the repo for commits and gets triggered if one or more commits are found. Please note that this can be adjusted depending on the project needs and trigger can be updated such as to trigger based on new PRs.
• Upon completion of the build, the container image is pushed to eiffel project on Nordix Container Image Registry (Please reach out to @fdegir if you need account on the registry.)

Build process.

• When the job gets triggered, project repo is cloned and the last commit on the tip of master branch is checked out
• Since this is a container image build, the job directly proceeds with the command to start the build. The command used for the build is
  podman build --log-level error --tag registry.nordix.org/eiffel/eiffel-gerrit-herald:latest .
• Image tag is set to latest since this is a build against the tip of master branch
• The complete build script is available on Nordix CICD Repository.
• Upon completion of the build, the container image is pushed to eiffel project on Nordix Container Image Registry
• The image is available for anonymous users and can be pulled using either one of the commands below
  podman pull registry.nordix.org/eiffel/eiffel-gerrit-herald:latest
docker pull registry.nordix.org/eiffel/eiffel-gerrit-herald:latest

What we need from you is

• Look at the build log and let us know if you notice anything that looks strange
• Correct the build process or the command used for the project build if they are incorrect
• If possible, pull the image and verify it works
  podman pull registry.nordix.org/eiffel/eiffel-gerrit-herald:latest
docker pull registry.nordix.org/eiffel/eiffel-gerrit-herald:latest
• Provide your opinion about what type of builds your project needs - PR validation before merge, builds after merge, builds after release, periodic builds

and anything else you may think if.

Please note that upcoming phases will enable testing as well and we will reach out to you when the time comes.

Motivation

Setup builds for the project

Exemplification

N/A

Benefits

Project will have builds available

Possible Drawbacks

N/A

Description

The log message emitted when a required configuration file propery is unset isn't helpful:

07:33:58.542 [main] WARN com.axis.eiffel.gerrit.herald.Service - All values in the config file must be set. null

The message should obviously include the name of the missing property. Also, since it indicates a fatal error condition it shouldn't just be warning.

Motivation

Log messages should be clear and actionable.

Benefits

The service becomes easier to operate and debug.

Possible Drawbacks

None.

Description

This service is currently self-contained and uses a Redis cache to keep track of ids of past events. This is great from a dependency-reduction perspective but carries other limitations.

We should refactor the service to drop the Redis cache and the outbound RabbitMQ code and instead rely on REMReM Publish for publishing messages. Publish relies on REMReM Generate which has a lookup feature that allows it to look up event ids for links against the configured event repository and insert them in the event that a client wants to submit.

Motivation

I'm planning to use Eiffel as the source of truth for source control changes, including historical changes, i.e. there will be a significant backfill operation. This is partially driven by a desire to compare historical releases but also because I want this service to be able to produce correct data from the start. Right now, only events that Gerrit Herald has sent itself can ever be the target of PREVIOUS_VERSION links. Since some git repos change rarely it can take quite some time before all SCC events have a complete set of links. Backfilling the Redis cache would probably be a workaround.

Benefits

• Less code since the message sending interface can be simplified. Reliably sending messages has proven to be surprisingly bug-prone. This service can focus on reliably constructing correct events.
• Instead of relying on an additional service for persisting critical data (the Redis cache) we'll rely on the canonical event repository that probably any Eiffel-implementing organization would have anyway. We'd also depend on REMReM Generate & Publish, but they're stateless and therefore less of a problem.
• Simpler backfill operations.
• Greater consistency in published messages, affecting e.g. topic naming.
• New protocol releases (or eiffel-semantics bugfixes) require fewer deployments to take effect.

Possible Drawbacks

• Organizations deploying Gerrit Herald would also have to run REMReM Generate & Publish and a compatible event repository, reducing the independence of this service and possibly increasing the operational overhead. That said, as mentioned above those services would probably be used anyway.

Description

The current dockerfile has a few problems that are easy to address:

• It's a single-stage build that downloads Gradle at build time, resulting in a 958 MB Docker image even though the base image is 470 MB and the jar file produced by the build is 30 MB.
• There's no convenient way of configuring the JVM options. The java -jar eiffel-gerrit-herald-1.0.0.jar command is in the CMD statement which is easily replaced to e.g. add -D and -Xmx JVM options, but then you add a dependency to the exact jar filename in the deployment configuration. We can improve this by adding support for an environment variable whose contents is included in the Java command.

Motivation

Reducing the size of the Docker image is obviously a good thing. Providing a standard method of passing JVM options reduces coupling between the source repository and the deployment configuration.

Exemplification

I want to be able to set system properties for the JVM. This is currently inconvenient.

Benefits

See Motivation.

Possible Drawbacks

None, except a slightly increased dockerfile complexity.

Description

SLF4J is used for the logging, which is good. However, we're not declaring a dependency to that library but relying on it to arrive transitively. We're also not including a concrete logging library so the only reason we're getting logs at all is that eiffel-remrem-semantics includes logback (which I think is a bug in that library; see comment thread in eiffel-community/eiffel-remrem-semantics#69). This means that the inclusion of log4j.properties is misleading since that file is never used.

I think we should:

• Add an explicit dependency to SLF4J.
• Add an explicit dependency to logback in anticipation of it going away from eiffel-remrem-semantics.
• Replace log4j.properties with a file that logback will pick up (or just remove the example file).

Motivation

Logging is important and we should make it easy for people who deploy the service to change the logging configuration. Adding missing dependencies is good house keeping.

Description

The configuration properties are currently only loaded from a config.properties file in the jar file's resources (via getResourceAsStream()). There doesn't seem to be a way to externalize this file, i.e. the only way to change the default configuration (something all users will do) is to rebuild the jar file to include the desired configuration. This might be acceptable if you're following the readme file's instruction of cloning the git, modifying the configuration, and running docker-compose up, but that's not how most sites prefer to operate things.

There should be a way to load the properties from an alternate file. A common pattern is to read a system property that contains the file path (or URL) of the property file to load.

Motivation

Storing configuration files in the file system rather than welding them into the executable binary is standard practice for most production deployments.

Benefits

Eiffel Gerrit Herald would be a more viable option for users.

Possible Drawbacks

None.

Description

The channel declarations are always active. This rarely works in production settings since it requires the user to have the configure permission for the exchange. Normal users rarely have that permission. It's still useful to be able to declare exchange actively but there must be a configuration option to be able to do passive declarations instead.

Motivation

By always making active declarations Gerrit Herald will be unable to connect to many real-world RabbitMQ instances.

Possible Drawbacks

None.