Skip to content
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

Enrich service wait up/down errors with more debug info. #2740

Merged
merged 4 commits into from
Aug 31, 2023

Conversation

mitchell-as
Copy link
Contributor

@mitchell-as mitchell-as commented Aug 30, 2023

TaskDX-1954 Improve wait up/down and enrich content of svc wait up/down error message

This should help with rollbar reports.

This is based on work done in the https://github.com/ActiveState/cli/commits/green/upg_svc_wait.DX-1824 branch: https://github.com/ActiveState/cli/pull/2739/files

I omitted the wait up/down pacing (c1920bb) because that's additional complication for little gain from what little I understood of it. Maintenance going forward may be a burden.

This should help with rollbar reports.
@mitchell-as mitchell-as requested a review from daved August 30, 2023 20:58
@mitchell-as mitchell-as marked this pull request as ready for review August 30, 2023 20:58
Copy link
Contributor

@daved daved left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Naatan For this story, I agree that we should step back from the wait up/down pacing (new story filed here: https://activestatef.atlassian.net/browse/DX-2176), but I'm curious to bring up the third point in the AC:

Consider and possibly include having some flag or arg passed in to the service start call at system startup so that we can better understand the context of the reported errors.

I feel fairly strongly that this would be an exceedingly useful data point. However, we didn't discuss a design. If you agree, should we create a separate story or update the current one?

@Naatan
Copy link
Member

Naatan commented Aug 30, 2023

Could you guys please add your concerns with the ACs to the jira ticket and move it back to triage.

We should not be omitting AC's without conversation with either myself or Pete.

I have opinions, but need more context. Probably easier if we talk about this during stand.

@mitchell-as mitchell-as requested a review from daved August 31, 2023 19:36
p, nil, nil,
p,
[]*captain.Flag{
{Name: "startup", Value: &startup}, // differentiate between autostart and cli invocation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we have curated the help output for the service, but I suggest setting Hidden to true.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest renaming "startup" to autostart.

return runStart(out, "svc-start:cli")
argText := "svc-start:cli"
if startup {
argText = "svc-start:startup"
Copy link
Contributor

@daved daved Aug 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
argText = "svc-start:startup"
argText += "-autostart"

@@ -13,7 +13,7 @@ import (
var Options = autostart.Options{
Name: constants.SvcAppName,
LaunchFileName: constants.SvcLaunchFileName,
Args: []string{"start"},
Args: []string{"start", "--startup"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Args: []string{"start", "--startup"},
Args: []string{"start", "--autostart"},

p, nil, nil,
p,
[]*captain.Flag{
{Name: "startup", Value: &startup}, // differentiate between autostart and cli invocation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest renaming "startup" to autostart.

@mitchell-as mitchell-as requested a review from daved August 31, 2023 20:07
daved
daved previously approved these changes Aug 31, 2023
return runStart(out, "svc-start:cli")
argText := "svc-start:cli"
if autostart {
argText += "-autostart"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
argText += "-autostart"
argText += "svc-start:auto"

@mitchell-as mitchell-as merged commit 8ff3fb2 into version/0-41-0-RC1 Aug 31, 2023
6 checks passed
@mitchell-as mitchell-as deleted the mitchell/dx-1954-2 branch August 31, 2023 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants