Refactored requirement operation and added multi-target/multi-language compatibility #3487

Naatan · 2024-09-10T20:42:51Z

DX-1897 Refactor ExecuteRequirementOperation

A lot went into this one. More than I had anticipated, but less than I would've liked.

Overall this focusses on trying to simplify and streamline the logic for installing and uninstalling packages. Similar to Mitchell's original PR this separates the install and uninstall actions. It also separates platform installs/uninstalls from requirement operations altogether, since platforms are not requirements. Finally, I turned this into a top level runner rather than a runbit, because there is no need for command centric runners as all the behaviour is already covered.

Summary of side-effects:

Multiple install targets:
- We no longer print the actual package names we're searching for, instead we just say a generic "searching for packages"
  - Reasoning: Depending on how input is passed not all packages are actually searched for.
- The --revision flag has been dropped.
  - Reasoning: this flag is targeting a single package operation, but state install now supports multiple operations. The only way to specify a revision would be to use buildscripts.
Multiple languages:
- We can no longer normalize names, as this requires a namespace and we cannot determine the namespace without exact matching.
  - Means users have to give the exact ingredient name, casing and all.
Misc:
- Added an --expand flag to state manifest which will show requirements by their full namespace.
  - This is to give a user a tool to review namespaces in case they need to disambiguate their input for state uninstall (eg. multiple packages with the same name, but different namespace).
- Commit messages and install/uninstall success messages have been updated. Eg. instead of "Package added: pkgName" it will now say: "Added: namespace/to/pkgName".
  - Adding the namespace is intentional as the user will need to reason about them when working with multi-language projects or projects with private ingredients.

…requirement op runbit)

…d, and make ordering consistent

mitchell-as

At a high level this looks fine to me. Good job! Some small things to address before merging, though.

mitchell-as · 2024-09-13T18:29:28Z

activestate.yaml

@@ -48,6 +48,7 @@ constants:
 scripts:
  - name: install-deps-dev
    language: bash
+    standalone: true


I'm curious why this was added, and if this is needed for other activestate.yaml files with custom scripts.

Because I find often while testing that it re-sources my runtime because I invoked it with another state tool version (ie. the one I'm working on vs my installed state tool). And end of the day none of our scripts actually require the runtime, they were always standalone, just never marked as such.

mitchell-as · 2024-09-13T18:33:12Z

internal/locale/locales/en-us.yaml

@@ -1145,23 +1083,25 @@ err_exec_recursion:
    To allow recursion, set [ACTIONABLE]{{.V1}}=true[/RESET]
 package_ingredient_alternatives:
  other: |
-    No results found for search term "[NOTICE]{{.V0}}[/RESET]". Did you mean:
+    No results found for search term {{.V0}}. Did you mean:


I think we should keep this quoted.

It's still highlighted, but because it now takes multiple values it happens outside of the locale file:

cli/internal/runners/install/rationalize.go

Line 29 in 02ba527

names = append(names, fmt.Sprintf(`[ACTIONABLE]%s[/RESET]`, r.Requested.Name))

I'll mark similar comments as resolved, but if you want to iterate further on this we can do it via this comment (just to keep it simple).

internal/locale/locales/en-us.yaml

mitchell-as · 2024-09-13T18:45:55Z

internal/runbits/reqop_runbit/update.go

+	}
+
+	// Start runtime sourcing UI
+	if !cfg.GetBool(constants.AsyncRuntimeConfig) {


I think we're handling this conditional inside runtime_runbit.Update(), so you can remove it here.

This was not intentional on my part, I think I copied this from the old code. But reading through it now I think it makes sense to retain it here. The comment in Update says that we still want the actual solving logic to run so we can raise errors. But that solving logic has already ran in this case when we created the commit. So solving it again in the async use-case wouldn't really give us any benefit.

I'll add a comment explaining as much.

mitchell-as · 2024-09-13T19:07:43Z

pkg/platform/model/vcs.go

@@ -169,6 +174,18 @@ func (n Namespace) String() string {
 	return n.value
 }

+func ParseNamespace(ns string) Namespace {


Nice, I've been hoping for a function like this for a while now.

mitchell-as · 2024-09-13T19:08:50Z

pkg/platform/model/vcs.go

@@ -204,21 +221,24 @@ func NewNamespacePlatform() Namespace {
 	return Namespace{NamespacePlatform, "platform"}
 }

-const OrgNamespacePrefix = "private/"
-
 func NewOrgNamespace(orgName string) Namespace {


Nit: for consistency we should rename this NewNamespaceOrg() to match the other functions.

mitchell-as · 2024-09-13T19:10:00Z

test/integration/install_int_test.go

+	cp = ts.Spawn("install", "djang")
+	cp.Expect("No results found")
+	cp.Expect("Did you mean")
+	cp.Expect("language/python/djang")


Please double-check you mean to expect "djang" instead of "django".

mitchell-as · 2024-09-13T19:13:22Z

test/integration/package_int_test.go

-		cp.Expect(fmt.Sprintf("Operating on project %s/python3-pkgtest", user.Username))
-		cp.ExpectRe("(?:Package added|being built)", termtest.OptExpectTimeout(30*time.Second))
+		cp.Expect("Operating on project ActiveState-CLI/small-python")
+		cp.Expect("Added: language/python/requests", e2e.RuntimeSourcingTimeoutOpt)


We have async runtimes enabled, so normally we don't need timeouts like this. Could you add a comment (perhaps top-level) explaining why we're using timeouts here? (Including the functions below.)

Based on the diff, it looks like we're expecting a long "resolving dependencies" window due to the Platform needing to build packages, but you wouldn't know that from looking at this code by itself.

This was addressing solve times, not sourcing times. You're totally right for flagging this though. I've introduced a new RuntimeSolvingTimeoutOpt.

mitchell-as · 2024-09-13T19:14:01Z

test/integration/package_int_test.go

 func (suite *PackageIntegrationTestSuite) TestPackage_UninstallDoesNotExist() {
 	suite.OnlyRunForTags(tagsuite.Package)

-	ts := e2e.New(suite.T(), false)
+	ts := e2e.New(suite.T(), true)


Did you mean to leave this change in place?

Nope, great catch!

…ugging

# Conflicts: # test/integration/package_int_test.go

mitchell-as

Looks good, but there are some test failures to address. Then it's good to go!

…ch this file

Naatan · 2024-09-23T16:45:14Z

The only test failure appears to be a sporadic one unrelated to this PR. Gonna move on ahead.

Naatan added 15 commits September 6, 2024 09:39

Implemented package install runner that is self contained (not using …

d859109

…requirement op runbit)

Implemented new requirement opts for state platforms add

e5da35e

Use new runner for bundles install

2dacc37

Fix suggestions

60c7ee1

languages install now uses new runner

ac10a21

Implemented uninstall runner

589c236

Added --expand flag to state manifest

db3c6c6

Tests and error conditions for state uninstall

f72b5b1

Added convenience method to run termtest command in debugger

dabd14a

Enforce namespace type

a8e99ce

Bundle uninstall uses new runner

c2aea6b

Implemented new runner for state platforms remove

e8e9355

Fixed debug output not supporting multiple invocations of same comman…

7dc7511

…d, and make ordering consistent

Remove debug code

3f582c4

Drop requirement operation and clean up

448d343

github-actions bot changed the base branch from master to version/0-47-0-RC1 September 10, 2024 20:43

Add test for ParseNamespace

6b825d7

Naatan added Test: package Test: languages Test: platforms Test: bundle Test: critical labels Sep 10, 2024

Naatan added 8 commits September 10, 2024 14:47

Fix bundle uninstall not working

05927a6

Centralize commit error handling

c8ef37f

Added JSON output for state install

38b3f86

Add json output for uninstall

48418db

Added structured output for platforms add/remove

6fef453

Added standalone install test

d3339da

Drop redundant test code

1277e45

Fix assertions

c3efc42

Naatan added 6 commits September 12, 2024 10:15

Add back auto wildcarding

0021f03

Reduce API requests

e12c615

Remove unused var

74e7e6a

Fix assertion

6d344d9

Mark input errors

2520098

Increase search timeout

02ba527

Naatan requested a review from mitchell-as September 13, 2024 16:37

mitchell-as requested changes Sep 13, 2024

View reviewed changes

Naatan added 10 commits September 13, 2024 14:01

Add comment explaining async config condition

d43b31a

Drop badly worded but also useless comments

0fa03bf

Add comment for SpawnDebuggerWithOpts

9d7a94a

Remove unused function

b4ada37

Rename method to be consistent

70a6f30

Expectation can use full word

60bc059

Differentiate between solving and sourcing runtime timeouts

1ee70c9

Retaining dirs is by definition something we ought to only do for deb…

2f767aa

…ugging

Merge remote-tracking branch 'origin/version/0-47-0-RC1' into DX-1897

6dc93a5

# Conflicts: # test/integration/package_int_test.go

Add increased timeout

05e069c

Naatan requested a review from mitchell-as September 13, 2024 22:02

mitchell-as requested changes Sep 14, 2024

View reviewed changes

Naatan added 4 commits September 19, 2024 09:15

Fix test; not guaranteed to return django

7a0c29d

Satisfy newline checker which is apparently bugged cause I didn't tou…

ed949e5

…ch this file

Fix unit test using removed function

4b0cca8

Give more time for double solve

2ae56e0

Naatan requested a review from mitchell-as September 19, 2024 20:48

mitchell-as approved these changes Sep 23, 2024

View reviewed changes

Naatan merged commit fd95129 into version/0-47-0-RC1 Sep 23, 2024
7 of 8 checks passed

Naatan deleted the DX-1897 branch September 23, 2024 16:45

as-builds added a commit that referenced this pull request Sep 23, 2024

Merge branch version/0-47-0-RC1 to adopt changes from PR #3487

efc04c6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored requirement operation and added multi-target/multi-language compatibility #3487

Refactored requirement operation and added multi-target/multi-language compatibility #3487

Naatan commented Sep 10, 2024 •

edited by github-actions bot

Loading

mitchell-as left a comment

mitchell-as Sep 13, 2024

Naatan Sep 13, 2024

mitchell-as Sep 13, 2024

Naatan Sep 13, 2024 •

edited

Loading

mitchell-as Sep 13, 2024

Naatan Sep 13, 2024

mitchell-as Sep 13, 2024

mitchell-as Sep 13, 2024

mitchell-as Sep 13, 2024

mitchell-as Sep 13, 2024

Naatan Sep 13, 2024

mitchell-as Sep 13, 2024

Naatan Sep 13, 2024

mitchell-as left a comment

Naatan commented Sep 23, 2024

Refactored requirement operation and added multi-target/multi-language compatibility #3487

Refactored requirement operation and added multi-target/multi-language compatibility #3487

Conversation

Naatan commented Sep 10, 2024 • edited by github-actions bot Loading

mitchell-as left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naatan Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchell-as left a comment

Choose a reason for hiding this comment

Naatan commented Sep 23, 2024

Naatan commented Sep 10, 2024 •

edited by github-actions bot

Loading

Naatan Sep 13, 2024 •

edited

Loading