fix(version): bump up dgraph version in source

[joshua-goldstein]

Problem

Dgraph version is hard coded in source. Currently it is still at 2103. This should be bumped up to the latest version. Thanks to @MichelDiz for noticing this issue.

Solution

Added todo's because we probably should not be hard coding this into source. We already pass the version in via ldflags, we could potentially use this instead.

[skrdgraph]

Looks like some our backup worker code uses x.DgraphVersion .. good catch!

[coveralls]

Coverage increased (+26.7%) to 64.016% when pulling e1b39e6 on joshua/dg-version into 70b8bd0 on main.

[matthewmcneely]

Maybe we should add this to the "Doing a Release" doc, basically just making sure this value is correct when the release is cut. But yeah, I agree in principle that it really should be pulling from the environment.

[skrdgraph]

+1 ^ would it be possible to pull this info from the environment at build time instead?

[MichelDiz]

Do we already know how we use this variable? My theory is that we use this in the "upgrade" subprogram.

[matthewmcneely]

Actually this PR is not needed. The -X build tag in the Makefile here sets this variable at build time. @skrdgraph, I'm assuming the release pipeline still sets env DGRAPH_VERSION as expected.

Maybe a comment could be added just above to let future readers know that it's set at build time and/or set the value to sometime like 0? @skrdgraph, feel free to close this PR once you're satisfied the new release pipeline is setting up DGRAPH_VERSION correctly.

[skrdgraph]

Actually this PR is not needed. The -X build tag in the Makefile here sets this variable at build time. @skrdgraph, I'm assuming the release pipeline still sets env DGRAPH_VERSION as expected.

Maybe a comment could be added just above to let future readers know that it's set at build time and/or set the value to sometime like 0? @skrdgraph, feel free to close this PR once you're satisfied the new release pipeline is setting up DGRAPH_VERSION correctly.

yeah ^ that makes sense.

In the release pipeline we obtain the version like this and this gets passed to the build step like this

I know that the dgraph version on the generated binary shows what we pass in the release pipeline. But regarding this PR, there was something more to this - @MichelDiz & @joshua-goldstein - we are voting to close this ... unless you remember how this came up ....

[MichelDiz]

Guys, I think this is necessary. See

dgraph/x/x.go

Line 143 in 6f7a6d2

ManifestVersion = 2105

It was renamed to Manifest version. So this is related to internal files. In the case to Backup files.

The backward compatibility of the backup's manifest was broken by #7810, although the tool was added (#7815) that enables smooth migration of manifest.
This PR makes backup backward compatible, by updating the manifest(in-memory) after reading.

See the PR #7825

[MichelDiz]

@matthewmcneely as far I undestood in the PRs related to this var. The -X build tag is irrelevant to this. It is related to backups and upgrades. So I think this variable needs attentio everytime we have a big change in the code that migh be breaking. Manually attention I mean. As its use has not been documented, we need to understand and document it.

[joshua-goldstein]

@matthewmcneely @skrdgraph Please take a look at my inline comment in the PR. There is a file x/init.go that contains the strings that are set by ldflags. See here. But this version is not being used by the backup code. Backup code uses the hardcoded variable in x.go (see here). Only the capitalization differentiates the two variables.

[joshua-goldstein]

Guys, I think this is necessary. See

dgraph/x/x.go

Line 143 in 6f7a6d2

ManifestVersion = 2105

It was renamed to Manifest version. So this is related to internal files. In the case to Backup files.

The backward compatibility of the backup's manifest was broken by #7810, although the tool was added (#7815) that enables smooth migration of manifest.
This PR makes backup backward compatible, by updating the manifest(in-memory) after reading.

See the PR #7825

@MichelDiz This is a good catch, I wasn't aware this was renamed at some point... Looks like that PR was merged into slash so presumably this will come up as we merge commits from slash into main.

[matthewmcneely]

It was renamed to Manifest version. So this is related to internal files. In the case to Backup files.

@MichelDiz I think this tool (ee/updatemanifest) was removed, right?

[MichelDiz]

@matthewmcneely Not sure, I never saw that tool. The backup and upgrades feels like isolated codes(subcommands) that wasn't well documented for future maintenance.

[matthewmcneely]

Not sure, I never saw that tool. The backup and upgrades feels like isolated codes(subcommands) that wasn't well documented for future maintenance.

OK, but what I can verify is that the on the main branch with the current Makefile, that variable x.DgraphVersion is set by whatever is in the -X build flag.

[MichelDiz]

@matthewmcneely correct, but it is a different variable. This one DgraphVersion is an exported variable, the one you're mentioning is in init.go which is private variable. That private one is set via -ldflags. If they are different, must be a reason for this.

[matthewmcneely]

@matthewmcneely correct, but it is a different variable. This one DgraphVersion is an exported variable, the one you're mentioning is in init.go which is private variable. That private one is set via -ldflags. If they are different, must be a reason for this.

Ah right, sorry. So I guess the question remains, should this variable be set by an ldflag and not hardcoded? From what I can see, it's only used by worker/backup_ee.go::ProcessBackupRequest. Which I'm assuming we'd want that function to actually have a correct (current) version when it creates the backup Manifest.

[skrdgraph]

This was changed to ManifestVersion, this solves the current naming issues.

Please leave a comment while closing existing PRs / Issues (just for clarifications).