fix ui doesn't work when running backend without 'feature-version' option #1178
Conversation
|
[REVIEW NOTIFICATION] This pull request has not been approved. To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
pkg/apiserver/info/info.go
Outdated
|
|
||
| // The FeatureVersion value is "N/A" when running `make && bin/tidb-dashboard` | ||
| // The FeatureVersion value is "Unknown" when running `go run cmd/tidb-dashboard/main.go` | ||
| featureSupported := |
There was a problem hiding this comment.
It's interesting, why do we have these two different values? Is it possible to be simply "", which is the well-known default value for a missing string?
There was a problem hiding this comment.
The config.FeatureVersion is assigned to version.PDVersion default.
func Default() *Config {
return &Config{
DataDir: "/tmp/dashboard-data",
TempDir: "",
PDEndPoint: "http://127.0.0.1:2379",
PublicPathPrefix: defaultPublicPathPrefix,
ClusterTLSConfig: nil,
TiDBTLSConfig: nil,
EnableTelemetry: true,
EnableExperimental: false,
FeatureVersion: version.PDVersion,
}
}
and the version.PDVeresion default value is "Unknown".
// Version information. It will be overwritten by LDFLAGS.
var (
InternalVersion = "Unknown"
Standalone = "Unknown" // Unknown, Yes or No
PDVersion = "Unknown"
BuildTime = "Unknown"
BuildGitHash = "Unknown"
)
Its value is overwritten to "N/A" by LDFLAGS when building in dev mode.
LDFLAGS += -X "$(DASHBOARD_PKG)/pkg/utils/version.PDVersion=N/A"
Let me try to assign FEATURE_VERSION to "" if PDVersion is "N/A" and "Unknown".
There was a problem hiding this comment.
Can we unify the PDVersion and the FEATURE_VERSION? Maybe we can specify the pd version during development.
There was a problem hiding this comment.
How about not assigning version.PDVersion to Default()? Will it cause any problems?
There was a problem hiding this comment.
How about not assigning
version.PDVersiontoDefault()? Will it cause any problems?
I think it should be the same effect. we eventually need to use the version.PDVersion, before or later.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1178 +/- ##
==========================================
+ Coverage 29.13% 29.17% +0.03%
==========================================
Files 286 286
Lines 16941 16933 -8
Branches 646 646
==========================================
+ Hits 4936 4940 +4
+ Misses 11774 11762 -12
Partials 231 231
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
| if _, err := semver.NewVersion(version.PDVersion); err != nil { | ||
| log.Fatal("Invalid sematic PD Version", zap.Error(err), zap.String("pd_version", version.PDVersion)) | ||
| } |
There was a problem hiding this comment.
Check the PDVersion when starting, expose the error as early as possible.
|
@baurine: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
close #1177
Try to dismiss the
FEATURE_VERSIONconception, use PDVersion directly to reduce the understanding confusion.