Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

[fix] Makefile fallback to using latest semver tag #526

Merged
merged 1 commit into from
Jun 9, 2022

Conversation

phoinixgrr
Copy link
Contributor

@phoinixgrr phoinixgrr commented Jun 9, 2022

Summary

Rationale

In case there is no Tag available, DIST_VER remains unitilized therefore building with an empty DIST_VER variable.
For example:

Build Linux amd64
env GOOS=linux GOARCH=amd64 go build -trimpath -ldflags '-X github.com/mattermost/mmctl/v6/commands.gitCommit=e58015f66734b77f223592a63d158652e94d5b82 -X github.com/mattermost/mmctl/v6/commands.gitTreeState="clean" -X github.com/mattermost/mmctl/v6/commands.buildDate='2022-05-31T08:20:04Z' -X github.com/mattermost/mmctl/v6/commands.Version=' -mod=vendor
tar cf build/linux_amd64.tar mmctl
md5sum < build/linux_amd64.tar | cut -d ' ' -f 1 > build/linux_amd64.tar.md5.txt
Build Linux arm64
env GOOS=linux GOARCH=arm64 go build -trimpath -ldflags '-X github.com/mattermost/mmctl/v6/commands.gitCommit=e58015f66734b77f223592a63d158652e94d5b82 -X github.com/mattermost/mmctl/v6/commands.gitTreeState="clean" -X github.com/mattermost/mmctl/v6/commands.buildDate='2022-05-31T08:20:04Z' -X github.com/mattermost/mmctl/v6/commands.Version=' -mod=vendor
tar cf build/linux_arm64.tar mmctl

In the above example, github.com/mattermost/mmctl/v6/commands.Version is empty.
And builds the following binary:

$ mmctl version
mmctl:
Version:
GitCommit:	e58015f66734b77f223592a63d158652e94d5b82
GitTreeState:	"clean"
BuildDate:	2022-05-31T08:20:04Z
GoVersion:	go1.17
Compiler:	gc
Platform:	linux/amd64

Proposal

In case DIST_VER is not defined, fallback to using latest semver tag available, instead of building with NULL DIST_VER

Ticket Link

Ticket: https://mattermost.atlassian.net/browse/DOPS-1049

@phoinixgrr phoinixgrr requested review from a user, spirosoik and isacikgoz June 9, 2022 09:24
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
In case DIST_VER is not defined, fallback to using latest semver tag available,
instead of building with NULL DIST_VER

Signed-off-by: Akis Maziotis <akis.maziotis@mattermost.com>
@phoinixgrr phoinixgrr requested a review from a user June 9, 2022 09:44
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 Nice work Akis

Copy link
Member

@isacikgoz isacikgoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@isacikgoz isacikgoz added the 4: Reviews Complete All reviewers have approved the pull request label Jun 9, 2022
@phoinixgrr phoinixgrr merged commit 0ba491d into master Jun 9, 2022
@phoinixgrr phoinixgrr deleted the makefile/fix/dist_ver branch June 9, 2022 12:37
phoinixgrr added a commit that referenced this pull request Jun 9, 2022
In case DIST_VER is not defined, fallback to using latest semver tag available,
instead of building with NULL DIST_VER

Signed-off-by: Akis Maziotis <akis.maziotis@mattermost.com>
@agnivade
Copy link
Member

@phoinixgrr - We have an opportunity to create a dot release for mmctl. Let's use this to backport this one as well.

/cc @amyblais

@amyblais amyblais added the CherryPick/Approved Meant for the quality or patch release tracked in the milestone label Jun 17, 2022
@amyblais
Copy link
Member

/cherry-pick release-7.0

@mattermod
Copy link
Contributor

Cherry pick is scheduled.

@mattermod
Copy link
Contributor

Error trying doing the automated Cherry picking. Please do this manually

fatal: No such remote 'upstream'

+++ Returning you to the master branch and cleaning up.

@amyblais
Copy link
Member

Needs a manual cherry-pick.

@amyblais
Copy link
Member

I think this is already in 7.0 branch https://github.com/mattermost/mmctl/commits/release-7.0.

@amyblais amyblais removed the CherryPick/Approved Meant for the quality or patch release tracked in the milestone label Jun 17, 2022
@agnivade
Copy link
Member

Yeah I see that. But is mmctl compiled with that fix? Because from the report here: #528 (comment), the version string isn't showing.

cc @phoinixgrr

@phoinixgrr
Copy link
Contributor Author

I did cherrypick that manually 8 days ago.
Was the 7.0 release compiled before or after that cherry-pick ?

@agnivade
Copy link
Member

Looking at this report: #528 (comment). The git commit sha contains your fix, but still the version is empty. Looks like we need to look at this again.

@agnivade
Copy link
Member

Actually, without your fix, none of the mmctl commands will work. Because all commands run a pre-check to compare the version.

@phoinixgrr
Copy link
Contributor Author

Thanks for the heads up @agnivade
Will investigate this further and revert.

@amyblais
Copy link
Member

@phoinixgrr I've opened a ticket here https://mattermost.atlassian.net/browse/DOPS-1090.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants