-
Notifications
You must be signed in to change notification settings - Fork 452
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
Unified Queue: implement correct fleet-initiated flag and setup experience priority #25448
base: feat-upcoming-activites-queue
Are you sure you want to change the base?
Unified Queue: implement correct fleet-initiated flag and setup experience priority #25448
Conversation
5c718c0
to
9426701
Compare
…leet-initiated-and-priority-4
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.
Looks good. Comments can be addressed in a future PR.
@@ -812,40 +812,45 @@ VALUES | |||
return "", ctxerr.Wrap(ctx, err, "getting installer data") | |||
} | |||
|
|||
fleetInitiated := !opts.SelfService && opts.PolicyID != 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.
nit: I would refactor this into: opts.IsFleetInitiated()
and inline that call into line 833.
@@ -812,40 +812,45 @@ VALUES | |||
return "", ctxerr.Wrap(ctx, err, "getting installer data") | |||
} | |||
|
|||
fleetInitiated := !opts.SelfService && opts.PolicyID != nil | |||
var priority int | |||
if opts.ForSetupExperience { |
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.
nit: refactor into opts.Priority()
?
fleetInitiated := !opts.SelfService && opts.PolicyID != nil | ||
var priority int | ||
if opts.ForSetupExperience { | ||
// a bit naive/simplistic for now, but we'll support user-provided | ||
// priorities in a future story and we can improve on how we manage those. | ||
priority = 100 | ||
} |
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.
Code duplication. See above comments.
#23914
Checklist for submitter
SELECT *
is avoided, SQL injection is prevented (using placeholders for values in statements)