REALMC-9363 - Removed client-side transpilation. #178

gagik · 2021-06-30T18:51:31Z

Replaced POST endpoint with the new PUT endpoint.
Removed client-side transpilation.
Modified the tests to be compatible with the changes.

internal/cloud/realm/dependencies.go

internal/cloud/realm/dependencies_test.go

internal/local/dependencies.go

internal/local/dependencies_test.go

internal/cloud/realm/dependencies.go

arahmanan

Good start! 💪

internal/cloud/realm/dependencies.go

internal/local/dependencies.go

internal/local/dependencies_test.go

internal/cloud/realm/dependencies_test.go

gagik · 2021-07-02T20:49:58Z

Still WIP just want to see how it runs without PrepareDependencies.

arahmanan · 2021-07-02T21:32:34Z

Still WIP just want to see how it runs without PrepareDependencies.

btw you can run evergreen patches through this command line tool as well.

makesitgo

I think you'll need to make sure we're updating tests for app diff and push here as well

let me know if you have any questions/thoughts about my comments here, would be happy to discuss anything further.

i am incredibly excited to see we can remove all things transpiler-related from this codebase though! nice work so far

internal/cloud/realm/client.go

internal/cloud/realm/dependencies.go

internal/commands/app/diff.go

internal/commands/push/command.go

internal/cloud/realm/dependencies_test.go

gagik

Just some things I am not so sure about.

internal/commands/push/command.go

internal/commands/push/command_test.go

internal/commands/push/command.go

install.js

internal/commands/push/command.go

internal/commands/push/command_test.go

makesitgo

lookin good here, just have another batch of comments/questions for you. really nice turnaround on a lot of this stuff!

.evg.yml

makesitgo · 2021-07-07T15:43:57Z

internal/cloud/realm/client.go

@@ -30,6 +30,7 @@ type Client interface {
 	ImportDependencies(groupID, appID, uploadPath string) error
 	Diff(groupID, appID string, appData interface{}) ([]string, error)
 	DiffDependencies(groupID, appID, uploadPath string) (DependenciesDiff, error)
+	DependenciesInstallation(groupID, appID string) (DependenciesInstallation, error)


thoughts on just naming this DependenciesStatus (or DependenciesInstallationStatus)? the latter is a bit wordy, but I think it might help convey that this client method is getting status information... I don't have a strong opinion on this one, I'm just having a hard time reasoning what an "installation" means

what about DependenciesImportStatus or DependenciesInstallStatus? DependenciesImportStatus seems to make most sense to me considering this is only being used for ImportDependencies right now and installation is technically related to the import as a whole? It's a bit weird how the process essentially starts as an import and ends as an install. @arahmanan

my one caution here would be that we shouldn't name the client method based on how its used, we should name it for what it does.

I finally got up to speed took a look over on the baas side... I would vote one of the following:

keep it as DependenciesInstallation ... I didn't realize that's how we referred to the struct in the backend, I can figure out how to reason what an installation means eventually

use DependenciesStatus since that's what the API handler is called (getDependenciesStatus)

I think ultimately I like the latter, although then it introduces a stutter with the Status field (which could additionally be renamed to State or Type while keeping the json mapping the same "status")

at the very least, I think this encourages us to remove the type DependencyStatus string as its overloading more of these words, and really doesn't add anything that a simple string couldn't do

on a different note I guess simply naming it DependenciesStatus makes sense considering it's a request to the dependencies/status endpoint so seems most fitting for that.

I don't have a strong opinion on this. I am also in favor of calling this function DependenciesStatus. I might also rename that in baas if it leads to confusion.

internal/cloud/realm/dependencies.go

internal/commands/push/command_test.go

gagik

Hopefully this addresses many of the issues so far.

gagik · 2021-07-07T20:44:06Z

internal/commands/push/command.go

@@ -198,7 +198,35 @@ func (cmd *Command) Handler(profile *user.Profile, ui terminal.UI, clients cli.C
 		if err := clients.Realm.ImportDependencies(appRemote.GroupID, appRemote.AppID, uploadPathDependencies); err != nil {
 			return err
 		}
-		ui.Print(terminal.NewTextLog("Uploaded dependencies archive"))
+
+		ui.Print(terminal.NewTextLog("Uploaded dependencies"))


Is this message necessary? If so, I thought it would be good to make it more consistent with the "Installed dependencies" message that should occur after it.

It does seem like it can go away now... it previously existed to mark the completion of this block, which as you allude to is now done with the "Installed dependencies" message below

I think the key here is we want to make sure this behaves similarly (read: prints the same amount of logs) as the hosting and/or reset cdn blocks do below (which from the looks of it appears to be the case)

Have you tested this locally yet? I'm guessing there's a test case below that can also confirm this...

Yes, as you saw below in the tests it ends up being 2 dependencies-related messages in the tests so I will remove it.

gagik · 2021-07-07T20:46:20Z

internal/cloud/realm/dependencies_test.go

+
+		// wait for dependencies to finish installation
+		var currentStatus realm.DependenciesStatus
+		for i := 0; i < 50; i++ {


in my computer it usually takes around <15 runs but I imagine with all sorts of delays and all, 50 seems to be a good balance between making sure it isn't flaky but also not running for too long; could be lowered.

gagik · 2021-07-07T20:48:48Z

internal/cloud/realm/dependencies.go


 	paramFile = "file"
 )

+// DependenciesStatus is used to get information from a dependencies status request
+type DependenciesStatus struct {
+	State   string `json:"status"`


I think this may be the best way to avoid stutter but I can see how the idea of a State of Status is a bit odd.

gagik · 2021-07-07T20:49:20Z

internal/cloud/realm/dependencies.go

+
+// set of known dependencies status states
+const (
+	DependenciesStateCreated    = "created"


I updated these to make them more related to the status.State as opposed to the status itself.

makesitgo

last few things from me, but this is lookin 🔥

.evg.yml

internal/cloud/realm/dependencies_test.go

makesitgo · 2021-07-08T13:20:35Z

internal/commands/push/command.go

@@ -198,7 +198,35 @@ func (cmd *Command) Handler(profile *user.Profile, ui terminal.UI, clients cli.C
 		if err := clients.Realm.ImportDependencies(appRemote.GroupID, appRemote.AppID, uploadPathDependencies); err != nil {
 			return err
 		}
-		ui.Print(terminal.NewTextLog("Uploaded dependencies archive"))
+
+		ui.Print(terminal.NewTextLog("Uploaded dependencies"))


It does seem like it can go away now... it previously existed to mark the completion of this block, which as you allude to is now done with the "Installed dependencies" message below

I think the key here is we want to make sure this behaves similarly (read: prints the same amount of logs) as the hosting and/or reset cdn blocks do below (which from the looks of it appears to be the case)

Have you tested this locally yet? I'm guessing there's a test case below that can also confirm this...

internal/commands/push/command.go

internal/commands/push/command_test.go

internal/commands/push/command.go

makesitgo

just one final question I'd like for you to answer via local testing, lmk if you want any help in setting that up

otherwise this looks terrific, really nice work here

🆒 beans

makesitgo · 2021-07-08T18:34:01Z

internal/commands/push/command.go

+				if err != nil {
+					return err
+				}
+				s.Suffix = fmt.Sprintf(" Installing dependencies: %s...", status.Message)


[question] maybe you can try to test this out (either by running the command and watching closely or mocking the realm client to suit your needs), but do we experience any "flicker" on this spinner/message when it's being set on every for loop iteration?

i ask b/c if so, then maybe we only want to be calling s.Suffix = ... if/when status.Message changes (which would then force us to remove the var err error declaration and use something like s, err := clients.Realm.DependenciesStatus(...) so we can have reference to both past and current statuses

I did not see any flicker when doing my testing so I don't think it's a problem. Unrelated to this, but I did notice a case of (spinner) Installing dependencies: ... when testing commands but this is because of the mock functions not providing any status messages during some tests. I could add status messages to the mock functions if this matters for tests. Previously when I ran tests with an actual server and a spinner it'd always have some sort of status message i.e. processing so for the CLI as a whole this is not a problem.

Awesome, and I wouldn't both with crafting messages for the tests, that text doesn't persist anyways (we don't assert on it)

I'd say all of this is good to go then!

gagik commented Jun 30, 2021

View reviewed changes

internal/cloud/realm/dependencies.go Outdated Show resolved Hide resolved

internal/cloud/realm/dependencies_test.go Show resolved Hide resolved

internal/local/dependencies.go Outdated Show resolved Hide resolved

gagik requested a review from arahmanan June 30, 2021 18:56

gagik force-pushed the REALMC-9363 branch from 997ddaa to ef64b9b Compare June 30, 2021 18:58

REALMC-9363 - Removed client-side transpilation.

6067b28

gagik force-pushed the REALMC-9363 branch from ef64b9b to 6067b28 Compare June 30, 2021 18:58

gagik marked this pull request as draft June 30, 2021 18:59

Update dependencies test to account for server-side transpilation.

f0350b2

gagik commented Jun 30, 2021

View reviewed changes

internal/local/dependencies_test.go Outdated Show resolved Hide resolved

gagik marked this pull request as ready for review June 30, 2021 20:06

gagik commented Jul 1, 2021

View reviewed changes

internal/cloud/realm/dependencies.go Outdated Show resolved Hide resolved

arahmanan reviewed Jul 2, 2021

View reviewed changes

arahmanan requested a review from makesitgo July 2, 2021 17:18

gagik marked this pull request as draft July 2, 2021 20:46

Remove preparing step and minor feedback changes.

02393e5

makesitgo suggested changes Jul 6, 2021

View reviewed changes

Remove transpiler installation code and feedback changes.

1196194

gagik commented Jul 6, 2021

View reviewed changes

internal/cloud/realm/dependencies_test.go Outdated Show resolved Hide resolved

gagik commented Jul 6, 2021

View reviewed changes

gagik marked this pull request as ready for review July 6, 2021 21:44

arahmanan reviewed Jul 7, 2021

View reviewed changes

gagik requested a review from makesitgo July 7, 2021 16:12

makesitgo reviewed Jul 7, 2021

View reviewed changes

gagik commented Jul 7, 2021

View reviewed changes

makesitgo reviewed Jul 8, 2021

View reviewed changes

Refactored functions and testing from feedback

2b5b295

gagik force-pushed the REALMC-9363 branch from e583c1b to 2b5b295 Compare July 8, 2021 14:43

makesitgo approved these changes Jul 8, 2021

View reviewed changes

gagik merged commit 90c58fd into 10gen:master Jul 8, 2021

gagik deleted the REALMC-9363 branch July 8, 2021 20:30

makesitgo mentioned this pull request Sep 22, 2021

REALMC-10294: Fix regression to allow uploading dependencies from node_modules directory #211

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REALMC-9363 - Removed client-side transpilation. #178

REALMC-9363 - Removed client-side transpilation. #178

gagik commented Jun 30, 2021

arahmanan left a comment

gagik commented Jul 2, 2021

arahmanan commented Jul 2, 2021

makesitgo left a comment

gagik left a comment

makesitgo left a comment

makesitgo Jul 7, 2021

gagik Jul 7, 2021 •

edited

Loading

makesitgo Jul 7, 2021 •

edited

Loading

gagik Jul 7, 2021

arahmanan Jul 7, 2021

gagik left a comment

gagik Jul 7, 2021

makesitgo Jul 8, 2021

gagik Jul 8, 2021

gagik Jul 7, 2021

gagik Jul 7, 2021

gagik Jul 7, 2021

makesitgo left a comment

makesitgo Jul 8, 2021

makesitgo left a comment

makesitgo Jul 8, 2021

gagik Jul 8, 2021 •

edited

Loading

makesitgo Jul 8, 2021

REALMC-9363 - Removed client-side transpilation. #178

REALMC-9363 - Removed client-side transpilation. #178

Conversation

gagik commented Jun 30, 2021

arahmanan left a comment

Choose a reason for hiding this comment

gagik commented Jul 2, 2021

arahmanan commented Jul 2, 2021

makesitgo left a comment

Choose a reason for hiding this comment

gagik left a comment

Choose a reason for hiding this comment

makesitgo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gagik Jul 7, 2021 • edited Loading

Choose a reason for hiding this comment

makesitgo Jul 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gagik 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makesitgo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makesitgo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gagik Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gagik Jul 7, 2021 •

edited

Loading

makesitgo Jul 7, 2021 •

edited

Loading

gagik Jul 8, 2021 •

edited

Loading