The -snapshot suffix should be added by the build system rather than hardcoded in commits

[spalger]

Updated description:

The -snapshot suffix should be added by default by the build system rather than being hardcoded into package.json, and you should be able to tell the build system not to apply it in a build if you pass --release. In addition, the default behavior of the build task should be to create os-packages as well. This is pretty consistent with how the Elasticsearch build process works and would make our build process compatible with the unified release process.

In addition, the build process should support the following options for our own benefit:

--skip-os-packages  # does not create os-packages, which is necessary on windows/os x
--skip-archives # does not create the .zip or .tar.gz files

Original description:
In an attempt to build a more automated release process, the Kibana build scripts need to support programmatically specifying the version assigned to the build output.

This would look something like this:

npm run build -- --version=5.0.0-beta2

Currently this version is read from the package.json file, and updating it requires pushing code. This has worked out nicely since we have been doing the release process by hand, but if we want to automate it we should make things a tad more dynamic.

I suggest that we do this:

• remove the version field from package.json
• update Kibana to assume the version number "0.0.0-snapshot" when no version field is found
• accept a --version flag in the build scripts
• write the --version flag to the package.json file during the build process
• update Jenkins to set the --version=${BRANCH}-snapshot flag when building the regular snapshots

[rjernst]

@this will really help automating the build process. In Elasticsearch, we do keep the version in a file, but it does not contain "snapshot". However, when doing a normal build with eg gradle assemble, the artifacts produced on master would have a version of 5.0.0-SNAPSHOT in their filenames. Snapshots for us are now part of the metadata/maven version, but not the version of elasticsearch itself (ie the code doesn't really know anything about snapshots). The metadata we keep in our jars does note whether the build was a snapshot, and we display this information in the root rest handler. The advantage to all this is building a release no longer requires changing a file, we just add a property to the build, like gradle -Dbuild.snapshot=false assemble.

[rashidkpc]

I'm +1 on this

[ycombinator]

@spalger I'd like to suggest a couple of differences from your original proposal above:

• Make the command-line argument to grunt build be --kibana-version (as opposed to --version). This is simply because --version is reserved by grunt to print out its own versions :)
• Keep the version field in the source package.json file and set it to 0.0.0-snapshot (as opposed to removing this field from this file altogether). Grunt will default to this value unless --kibana-version is set when invoking grunt build. I don't have a strong rationale for this one, except that the source package.json file seems like an intuitive and easy-to-find place for the default version, as opposed to being buried in the grunt task code.

[spalger]

@ycombinator 👍 I too think that if we can put a version in package.json we should, and 0.0.0-snapshot seems as like the best worst option.

[epixa]

The biggest problem I see with this change is going to be how version matching on plugins is going to be handled. There's a 5.0 ticket open to make plugin installation dependent on patch level versions, so we'll need to somehow make sure that plugins can still be installed on master.

[rjernst]

If you are going to write the version to package.json during the build anyways, why not have the version in a file, and use a flag to indicate whether it is a snapshot like ES does? Would that allow everything else to work as it does now (ie package.json would be no different, it would always contain the version, because the build would always insert it)?

[rjernst]

What's happening here? Is there something blocking this?

[ycombinator]

I think @epixa's comment above needs to be addressed. I haven't had time to look into it (been busy with other issues). @spalger do you have any thoughts?

[epixa]

There are some minor details to sort out (like the plugin version matching that I mentioned above), but the only significant blocker at this point is having the time to do it.

[w33ble]

we'll need to somehow make sure that plugins can still be installed on master

The only proposal for fixing this so far is by Ryan, who suggested we just save the version in another file. I don't see how that helps much though, if we're going to write it to a file anyway it might as well just stay in package.json. Are there other ideas? Maybe we simply don't do plugin version checking in dev mode? Or change the behavior to a warning instead of a fatal error?

We also have specific version callouts in the readme (and maybe some other files) that I don't see addressed in the original proposal. How are we going to handle these? Maybe we can just remove specific versions from them?

[epixa]

How about we just set version inside package.json to be a definitive version just like Elasticsearch does, and then we add a snapshot flag to the build system? Then the version that actually gets added to the package.json in the final builds includes -snapshot only if the flag is passed. This seems to be consistent with how ES does it as well.

[rjernst]

That would be perfect. But I would do a release flag so snapshot is included by default, that is what ES does.

[epixa]

Is that a straight up boolean flag, or does ES allow for setting alpha or beta?

[rjernst]

alpha and beta are part of the version number. It is only a flag that indicates whether -SNAPSHOT should exist.

[epixa]

Gotcha. That'd work for me.

[epixa]

My current plan of action is to change our build/release process to essentially be the following:

build --release --skip-os-packages --skip-archives --version=5.0.0-wat
release --release --skip-os-packages --skip-archives

All flags would be optional, and you would no longer access ospackages or publishpackages stuff directly.

Anyone have any thoughts?

[w33ble]

Seems reasonable. --release drops SNAPSHOT I take it? If you use both --version and --release, does the release flag just get ignored?

[epixa]

Oh, I forgot to add: the source code would no longer have any reference to -snapshot, and the build process would take care of appending -snapshot to version.

--release would tell the build process not to append -snapshot to version
--version would just be an overwrite for the version defined in the source package.json

And now that I write that out, I can't think of a reason to have --version at all. Builds must be associated with a specific commit, and that commit should have the same version as package.json.

It should also be possible to upload a --release build in a staged fashion (like ES does), and that should be sorted out as well.

Back to the drawing board.

[rjernst]

There's no need to mess with uploading anything, the unified release will handle all that. We just need the --release flag.

[w33ble]

Oh, I forgot to add: the source code would no longer have any reference to -snapshot, and the build process would take care of appending -snapshot to version.

I dig that.

The --version flag may be a nice option to have, in case you want to make a one-off build and not confuse it with some official thing, or otherwise mark it (like with the commit or something).

[epixa]

Is the unified release process to handle snapshot builds on all pushes to master and such?

On May 25, 2016, at 5:20 PM, Ryan Ernst notifications@github.com wrote:

There's no need to mess with uploading anything, the unified release will handle all that. We just need the --release flag.

—
You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub

[rjernst]

Yes, it will handle building and publishing snapshots from master.

[epixa]

I have an implementation for this in #7317, though I haven't done enough testing on it yet to assign it for review.

[epixa]

There's an issue with this approach that will need to be resolved before we can move forward: the development workflow for Kibana does not involve creating builds, so the version in development will not include -snapshot. This triggers the check in Kibana that verifies that the current install has the same version as the Elasticsearch it is running against since the ES version does include -snapshot. For the version check, we can't just ignore suffixes because we do need to catch things like -alpha3.

I'm not sure of a good solution for this at the moment, but I'll keep thinking about it. In the meantime, ideas welcome!

[w33ble]

I'm not sure of a good solution for this at the moment, but I'll keep thinking about it. In the meantime, ideas welcome!

The server knows if it's in dev mode, no? Why are we doing string parsing when we can just check that mode?

[epixa]

Do you mean we should just not do version checking when running in dev mode?

[w33ble]

That's not what I meant, we still need to enforce the version partially I believe. What I meant was that whatever is happening logically when it sees -snapshot can instead happen when in dev mode. It's not a full 1:1 use case comparison (dev mode wasn't always required previously to side-step the version matching), but I think the dev mode check covers most of our needs.

[Bargs]

I'd be wary of relying on dev mode. I work outside of dev mode a lot when I need to debug node or run API tests, I wouldn't want that to become even more difficult.

[epixa]

I think I may just bake in -snapshot as a special case to ignore.

[epixa]

I'm actually incorrect about the root cause of the issue I encountered. #7278 is the real issue I was running into. If that doesn't get resolved before the the build change in this issue gets merged, then everyone will need to wipe out their local esvm and start fresh (or at least kill .kibana).

[epixa]

Alright, I think the build changes in #7317 are ready for review.