Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

passing version configuration as part of the build to fix cli version command #3345

Merged
merged 4 commits into from
Sep 13, 2023

Conversation

enekofb
Copy link
Contributor

@enekofb enekofb commented Sep 12, 2023

Closes #3343

What changed?

  • Passing Ldflags for gitops cli so variable are updated with the right values instead of default.
  • Removed the bits around getting configuration from weave gitops as it seems that was only used here (and we dont require it)

Why was this change made?

  • Address bug

How was this change implemented?

  • Using the same approach used for other Ldflags within the Makefile

How did you validate the change?

  • Explain how a reviewer can verify the change themselves

checkout the branch and repeat the following steps

➜  weave-gitops-enterprise git:(issues/3343) ✗ make cmd/gitops/gitops                                                                                                                                                    
CGO_ENABLED=0 go build -ldflags "-X github.com/weaveworks/weave-gitops/cmd/gitops/version.Branch=main -X github.com/weaveworks/weave-gitops/cmd/gitops/version.BuildTime=2023-09-12_16:36:21 -X github.com/weaveworks/weave-gitops/cmd/gitops/version.GitCommit=a8dee904b28d7087f8212f45e69654a9f7734136 -X github.com/weaveworks/weave-gitops/cmd/gitops/version.Version=v0.31.0-27-ga8dee90" -gcflags='all=-N -l' -o cmd/gitops/gitops  ./cmd/gitops

➜  weave-gitops-enterprise git:(issues/3343) ✗ cmd/gitops/gitops version                                                                                                                                                 Current Version: v0.31.0-27-ga8dee90
GitCommit: a8dee904b28d7087f8212f45e69654a9f7734136
BuildTime: 2023-09-12_16:36:21
Branch: main
  • Integration tests -- what is covered, what cannot be covered;
    or, explain why there are no new tests
  • Unit tests -- what is covered, what cannot be covered; are
    there tests that fail without the change?

Added test case to ensure we have some regression within the code

Release notes
No

Documentation Changes
No

Other follow ups
Not that i could think of

@enekofb enekofb added the bug Something isn't working label Sep 12, 2023
@enekofb enekofb force-pushed the issues/3343 branch 2 times, most recently from 5a0e8d9 to a60d453 Compare September 13, 2023 06:15
@enekofb enekofb marked this pull request as ready for review September 13, 2023 06:15
@enekofb enekofb force-pushed the issues/3343 branch 3 times, most recently from 0f80e53 to 51ce357 Compare September 13, 2023 07:35
Copy link
Contributor

@opudrovs opudrovs left a comment

Choose a reason for hiding this comment

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

And you even added tests for this, neat! ✨

@enekofb enekofb merged commit 904bf7e into main Sep 13, 2023
10 checks passed
@enekofb enekofb deleted the issues/3343 branch September 13, 2023 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enterprise gitops cli advertised as version v0.0.0
3 participants