CLI | Change Vorpal vorpal-latest-version flag flow (AST-48376) #797
Conversation
|
Policy Management Violations
No New Or Fixed Issues Found |
AlvoBen
changed the title
| if err := vorpalWrapper.HealthCheck(); err == nil { | ||
| _ = vorpalWrapper.ShutDown() | ||
| if !vorpalInstalled || vorpalParams.VorpalUpdateVersion { | ||
| if err := checkLicense(vorpalParams.IsDefaultAgent, vorpalWrappers); err != nil { |
There was a problem hiding this comment.
I think Or told us to shut down the service here
There was a problem hiding this comment.
we need to check here for helth check before shutting down. or the shut down check that there sis engine alive bofre>?
internal/services/vorpal.go
Outdated
| return err | ||
| } | ||
| if newInstallation && vorpalParams.VorpalUpdateVersion { |
There was a problem hiding this comment.
| if newInstallation && vorpalParams.VorpalUpdateVersion { | |
| if newInstallation { |
in a situation we check together that I removed the executable file- we also want to shut down the service
| @@ -50,38 +50,38 @@ func downloadFile(downloadURLPath, filePath string) error { | |||
| // InstallOrUpgrade Checks the version according to the hash file, | |||
| // downloads the RealTime installation if the version is not up-to-date, | |||
| // Extracts the RealTime installation according to the operating system type | |||
| func InstallOrUpgrade(installationConfiguration *InstallationConfiguration) error { | |||
| func InstallOrUpgrade(installationConfiguration *InstallationConfiguration) (bool, error) { | |||
There was a problem hiding this comment.
is it possible to add an alias to the bool? like - bool doesInstalltion
internal/services/vorpal.go
Outdated
| return err | ||
| } | ||
| if newInstallation { | ||
| _ = vorpalWrapper.ShutDown() |
There was a problem hiding this comment.
we are doing a shutdown in other flow before we are running a new one no? just need to make sure there are no bugs in this flow.
let's have tests please..we have many scenarios
There was a problem hiding this comment.
This function is the only place we are shutting down. This pr did not change this logic
There was a problem hiding this comment.
we didn't have shutdown aywehre else?
There was a problem hiding this comment.
same here for the shutdown and healthcheck
There was a problem hiding this comment.
No, this is the only function that we use shutdowns.
For healthcheck there are more places
By submitting a PR to this repository, you agree to the terms within the Checkmarx Code of Conduct. Please see the contributing guidelines for how to create and submit a high-quality PR for this repo.
Description
References
Testing
Checklist