-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Client AutoUpdate proto structure changes #47532
Changes from 1 commit
401d2a3
b9449c9
324813e
ca422aa
1e3b6a4
4544f49
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -34,7 +34,26 @@ message AutoUpdateConfig { | |||||
// AutoUpdateConfigSpec encodes the parameters of the autoupdate config object. | ||||||
message AutoUpdateConfigSpec { | ||||||
// ToolsAutoupdate encodes the feature flag to enable/disable tools autoupdates. | ||||||
// Deprecated: use AutoUpdateConfigTools instead. | ||||||
bool tools_autoupdate = 1; | ||||||
|
||||||
AutoUpdateConfigTools tools = 2; | ||||||
} | ||||||
|
||||||
// AutoUpdateConfigTools encodes the parameters for client tools auto updates. | ||||||
message AutoUpdateConfigTools { | ||||||
// Mode defines state of the client tools auto update. | ||||||
ToolsMode mode = 1; | ||||||
} | ||||||
|
||||||
// ToolsMode type for client tools enabling state. | ||||||
enum ToolsMode { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
// TOOLS_MODE_UNSPECIFIED is undefined mode. | ||||||
TOOLS_MODE_UNSPECIFIED = 0; | ||||||
// TOOLS_MODE_ENABLED client tools auto update is enabled. | ||||||
TOOLS_MODE_ENABLED = 1; | ||||||
// TOOLS_MODE_DISABLED client tools auto update is disabled. | ||||||
TOOLS_MODE_DISABLED = 2; | ||||||
} | ||||||
|
||||||
// AutoUpdateVersion is a resource singleton with version required for | ||||||
|
@@ -51,5 +70,14 @@ message AutoUpdateVersion { | |||||
// AutoUpdateVersionSpec encodes the parameters of the autoupdate versions. | ||||||
message AutoUpdateVersionSpec { | ||||||
// ToolsVersion is the semantic version required for tools autoupdates. | ||||||
// Deprecated: use AutoUpdateVersionTools instead. | ||||||
string tools_version = 1; | ||||||
|
||||||
AutoUpdateVersionTools tools = 2; | ||||||
} | ||||||
|
||||||
// AutoUpdateVersionTools encodes the parameters for client tools auto updates. | ||||||
message AutoUpdateVersionTools { | ||||||
// TargetVersion is the semantic version required for tools autoupdates. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this the target version we want you to upgrade to or is it the version required for you to be able to update to something else? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vapopov what's the correct version here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tigrato TargetVersion is mandatory version for update, if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we rename it to:
The way it's written today is misleading and reads as the version required for auto updates and not the target auto update version |
||||||
string target_version = 1; | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,11 +60,13 @@ func ValidateAutoUpdateVersion(v *autoupdate.AutoUpdateVersion) error { | |
return trace.BadParameter("Spec is nil") | ||
} | ||
|
||
if v.Spec.ToolsVersion == "" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you no longer validate v.Spec.ToolsVersion. Is it used in prod? Should we keep it if not used? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually we decided to rename before actual usage in prod, so this resource is not used previously |
||
return trace.BadParameter("ToolsVersion is unset") | ||
} | ||
if _, err := semver.NewVersion(v.Spec.ToolsVersion); err != nil { | ||
return trace.BadParameter("ToolsVersion is not a valid semantic version") | ||
if v.Spec.Tools != nil { | ||
if v.Spec.Tools.TargetVersion == "" { | ||
return trace.BadParameter("TargetVersion is unset") | ||
} | ||
if _, err := semver.NewVersion(v.Spec.Tools.TargetVersion); err != nil { | ||
return trace.BadParameter("TargetVersion is not a valid semantic version") | ||
} | ||
} | ||
|
||
return nil | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this being used in prod? If no, we can reserve the field and continue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 let's reserve the fields and deprecate