search

elastic / elastic-stack-installers

Windows MSI packages for Elastic stack
Apache License 2.0
2 stars 16 forks source link

Stack installers build should honour version of the manifest #271

Closed alpar-t closed 4 months ago

alpar-t commented 5 months ago

Right now, when stack installers are built, they read the version of the stack from Directory.Build.props both the build script as well as the [dra publish (https://github.com/elastic/elastic-stack-installers/blob/3142f718ee2f49fbc35d5a563ebc7b9016cfca07/.buildkite/scripts/dra-publish.sh#L23) do this.

When the build is triggered with a MANIFEST_URL parameter, e.g. when it's being triggered by the unified release, the manifest could be for a different release of the stack causing the build to fail. This happens around stack releases if a build is triggered when one of the jobs has a bumped version but not the other. This failure cascades to the unified release job. Since there are many projects that are part of the unified release we would like to avoid having to coordinate the order in which we do the version bumps when possible.

We can prevent these failures by always honouring the version from the passed in manifest e.g. curl https://snapshots.elastic.co/8.15.0-fd647d58/manifest-8.15.0-SNAPSHOT.json | jq .version and only use the version from Directory.Build.props when a MANIFEST_URL is not passed in.

jlind23 commented 5 months ago

@rowlandgeoff @dliappis isn't this something we can add on the Ingest Eng prod radar? cc @ycombinator

dliappis commented 5 months ago

@alpar-t I took a look at this and tried to refresh my memory looking at the code base to see what's the optimal path forward.

Currently, even if MANIFEST_URL isn't available, e.g. when a CI job is triggered with a PR rather than unified release, we generate it anyway in build.ps1. dra publish always constructs MANIFEST_URL from the version.

Therefore, I think what we need is simple and unless I am mistaken, we can completely remove the Directory.Build.props file and even the need to bump it for the branches main and 8.x.

@alpar-t could you take a look at this draft PR https://github.com/elastic/elastic-stack-installers/pull/274/files and see if I am missing something? Also @pierrehilbert I would appreciate it if you could think of anything that I am missing by removing the need to bump versions in Directory.Build.props (and the file itself).

pierrehilbert commented 5 months ago

I had a quick look and it seems that you covered everything. Thanks @dliappis