Remove headless #2835
Remove headless #2835
Conversation
MDrakos
commented
Oct 19, 2023
•
MDrakos
commented
 DX-2224 Remove headless commits
|
|
I've fixed the test failures that I introduced. Any further failures should be unrelated to this PR. |
internal/constants/constants.go
Outdated
| @@ -316,6 +313,7 @@ const ActiveStateSupportURL = "https://www.activestate.com/support/?utm_source=p | |||
| const ActiveStateDashboardURL = "https://platform.activestate.com/?utm_source=platform-application-gui&utm_medium=activestate-desktop&utm_content=drop-down&utm_campaign=maru" | |||
|
|
|||
| // DashboardCommitURL is the URL used to inspect commits | |||
| // TODO: Can this be removed? | |||
There was a problem hiding this comment.
I worry this TODO will get lost unless we address it now or file a story to investigate. A quick search yields this being used by pkg/platform/model/project.go's CommitURL() function, but that function does not appear to be called by anything. Perhaps remove this along with that function.
There was a problem hiding this comment.
Whoops, I meant to look into this before I opened a PR. You're right, looks like it can be removed.
| rtTarget = target.NewCustomTarget("", "", "", params.Path, trigger, true) | ||
| } else { | ||
| rtTarget = target.NewProjectTarget(proj, nil, trigger) | ||
| return locale.WrapInputError(err, "exec_no_project_at_path", "Could not find project file at {{.V0}}", projectDir) |
There was a problem hiding this comment.
Is this ultimately a runtime issue? If so, does rationalize/types.go's ErrNoProject apply here too?
There was a problem hiding this comment.
I'm going to opt to leave this one here for now. I think it is a candidate for ErrNoProject but this error is surfaced specifically when a user provides the project path to the command. When it comes time for this command to support user-facing errors we can enrich ErrNoProject like the localization is doing currently.
| @@ -90,7 +89,6 @@ func New(target setup.Targeter, an analytics.Dispatcher, svcm *model.SvcModel, a | |||
| recordAttempt(an, target) | |||
| an.Event(anaConsts.CatRuntimeDebug, anaConsts.ActRuntimeStart, &dimensions.Values{ | |||
| Trigger: ptr.To(target.Trigger().String()), | |||
| Headless: ptr.To(strconv.FormatBool(target.Headless())), | |||
There was a problem hiding this comment.
Thinking out loud: do we want to deprecate this dimension? We've done so in the past for other dimensions, but I don't know exactly what that entails.
There was a problem hiding this comment.
We've deprecated events in the past but I'm not sure about dimensions... I'll see what I can find. We want to keep the dimension around for now as there may be headless projects out in the wild.
| @@ -221,6 +221,13 @@ func (suite *BundleIntegrationTestSuite) TestJSON() { | |||
| cp.ExpectExitCode(0) | |||
| AssertValidJSON(suite.T(), cp) | |||
|
|
|||
| cp = ts.SpawnWithOpts( | |||
| e2e.OptArgs("checkout", "ActiveState-CLI/Bundles", "."), | |||
| e2e.OptAppendEnv("ACTIVESTATE_CLI_DISABLE_RUNTIME=false"), | |||
There was a problem hiding this comment.
I'm pretty sure we don't need to source the runtime here, as we're sourcing it in the next command. I think we can safely remove this line to cut down on execution time.
There was a problem hiding this comment.
Let me give it another shot. When I had this line disabled I was running into issues with the tests failing.
| @@ -501,6 +470,13 @@ func (suite *PackageIntegrationTestSuite) TestJSON() { | |||
| cp.ExpectExitCode(0) | |||
| AssertValidJSON(suite.T(), cp) | |||
|
|
|||
| cp = ts.SpawnWithOpts( | |||
| e2e.OptArgs("checkout", "ActiveState-CLI/Packages-Perl", "."), | |||
| e2e.OptAppendEnv("ACTIVESTATE_CLI_DISABLE_RUNTIME=false"), | |||
There was a problem hiding this comment.
I think we can remove this line because the runtime is being sourced in the next operation.
There was a problem hiding this comment.
Removing this line causes the tests to panic.
Removed unreferenced code Use rationalized error type Attempt to speed up integration tests
