-
Notifications
You must be signed in to change notification settings - Fork 93
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
upgrade service preflights api #4693
upgrade service preflights api #4693
Conversation
* add available updates api endpoint * rename pending to available
* save config api for upgrade service * update setProgress and setResults * remove TODO * Update pkg/upgradeservice/preflight/preflight.go Co-authored-by: Salah Al Saleh <[email protected]> * clean up result and progress funcs * remove run error from setResult func --------- Co-authored-by: Salah Al Saleh <[email protected]>
func hasFailingStrictPreflights(results *types.PreflightResults) bool { | ||
// convert to troubleshoot type so we can use the existing function | ||
uploadResults := &troubleshootpreflight.UploadPreflightResults{} | ||
uploadResults.Results = results.Results | ||
for _, e := range results.Errors { | ||
uploadResults.Errors = append(uploadResults.Errors, &troubleshootpreflight.UploadPreflightError{ | ||
Error: e.Error, | ||
}) | ||
} | ||
return troubleshootpreflight.HasStrictAnalyzersFailed(uploadResults) | ||
} |
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.
This function felt weird to write, but there are two types that are nearly identical PreflightResults
and UploadPreflightResults
, the latter of which is from troubleshoot. I wanted to keep using the same check for failing strict preflights, but if there's a better way to do this, I'm all ears.
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.
it's fine for now.
Progress string `json:"progress,omitempty"` | ||
Result *types.PreflightResult `json:"result"` |
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.
Modified this type to match more closely with what we store in the database
|
||
func ResetPreflightData() error { | ||
if err := os.RemoveAll(PreflightDataFilepath); err != nil { | ||
return errors.Wrap(err, "failed to remove preflight data") |
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.
maybe don't fail if it doesn't exist?
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.
I believe os.RemoveAll
will do this. Just switched it in dbd8cd0 for this same reason.
HasFailingStrictPreflights: hasFailingStrictPreflights(results), | ||
} | ||
// clear the progress once the results are set | ||
preflightData.Progress = "" |
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 pick: can this be inside the struct above?
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.
made a change in 7a4d7bb to address this one
a34f374
to
a8058b1
Compare
What this PR does / why we need it:
This PR adds two endpoints
POST /preflight/run
andGET /preflight/result
to support running preflights and fetching preflight results, respectively.Which issue(s) this PR fixes:
https://app.shortcut.com/replicated/story/105495/upgrade-service-api-side-of-the-preflights-page-in-the-deployment-wizard
Special notes for your reviewer:
Steps to reproduce
Does this PR introduce a user-facing change?
Does this PR require documentation?