Origin/suggest relevant envs #32

lovestaco · 2023-08-13T10:12:42Z

What type of MR is this?

Description

This MR adds the feature of providing environment variables relevant to input.
To provide relevant variables making use of trie.

Issue

Important files to start review from

controller.go

l2 --help

Usage:
  l2 [OPTIONS] [LamaAPIFile]

Application Options:
  -o, --output=      Path to output JSON file to store logs, headers and result
  -v, --verbose      Show verbose debug information
  -b, --prettify     Prettify specified .l2 file
  -c, --convert=     Generate code in given language and library (ex: python.requests); reference: tinyurl.com/l2codegen
  -n, --nocolor      Disable color in httpie output
  -u, --update       Update l2 binary to the latest released version (Linux/MacOS only)
  -p, --postmanfile= JSON export from Postman (Settings -> Data -> Export Data)
  -l, --lama2dir=    Output directory to put .l2 files after conversion from Postman format
  -h, --help         Usage help for Lama2
  -e, --env=         Get a JSON of environment variables revelant to input arg (default: UNSET)
      --version      Print Lama2 binary version

Help Options:
  -h, --help         Show this help message

Added tests?

👍 yes
🙅 no, because they aren't needed
🙋 no, because I need help

Added to documentation?

If documentation update is there then run `make mkdocs` once PR is in acceptable state.

📓 make mkdocs
📜 README.md
🙅 no documentation needed

Adding go-autocomplete-trie

shrsv · 2023-08-14T14:33:16Z

Please mention relevant issues in description, so that they get tracked, and eventually closed.

shrsv · 2023-08-14T14:37:20Z

controller/controller.go

+	}
+
+	if len(o.Env) > 0 {
+		envMap, err := preprocess.GetL2EnvVariables(dir)


Get envMap is common to both the if statements (this and the previous one). So, we should get it outside/before both the if statement

Yes extracted it out processEnvironmentVariables()

rogueloop · 2023-08-15T15:36:02Z

tests/env_command_test.go

@@ -130,3 +162,15 @@ func checkBHost(t *testing.T, envMap map[string]EnvData) {
 		}
 	}
 }
+
+func checkBHostDoesNotExist(t *testing.T, envMap map[string]EnvData) {
+	if _, ok := envMap["BHOST"]; ok {


What is BHOST / AHOST key ?

It is variables from env file

shrsv · 2023-08-15T15:43:34Z

Let's set default value to UNSET_VU5TRVQ just to avoid conflicts with a genuine value such as UNSET

shrsv · 2023-08-15T15:49:18Z

controller/controller.go

+func processEnvironmentVariables(o *lama2cmd.Opts, directory string) {
+	envMap, err := preprocess.GetL2EnvVariables(directory)
+	if err != nil {
+		log.Error().Str("Type", "Preprocess").Msg(err.Error())


Leave a comment highlighting what sort of error can be expected here after verification. I think it could only be a JSON marshalling error. Everything else returns an empty map.

shrsv · 2023-08-15T15:51:44Z

controller/controller.go

+		log.Error().Str("Type", "Preprocess").Msg(err.Error())
+		os.Exit(0)
+	}
+	if o.Env == "" { // -e=''


The order of these checks must be simplified:

Is it an -e invocation? ( != unset)

is it empty? handle

is it not empty? handle

Not an -e continue.

Also if it is a -e request, for efficiency sake, once printing is done, we should simply exit the app, right? We are not doing that now, continuing processing.

Essentially -e is a editor extension support command; we don't expect a normal user to poke into it.

shrsv · 2023-08-15T15:52:10Z

controller/controller.go

+	}
+}
+
+func marshalAndPrintJSON(data interface{}) {


Should go into utils, not clutter controller.

shrsv · 2023-08-15T15:52:34Z

controller/controller.go

+	os.Exit(0)
+}
+
+func getRelevantEnvs(envMap map[string]map[string]interface{}, o *lama2cmd.Opts) map[string]interface{} {


All these env related functionalities should goto its own file.

shrsv · 2023-08-15T15:56:02Z

tests/basic_commands_test.go

+func runL2CommandAndGetOutput(t *testing.T, cmdArgs ...string) string {
+
+	// Get the full path to the l2 binary
+	l2BinPath := "../build/l2"


How do we ensure check is happening on the latest binary? Any mechanism to avoid the error?

Yes every test will build a new binary before executing the tests

Locally when we run make test it builds new binary

shrsv · 2023-08-15T15:58:45Z

tests/basic_commands_test.go

+	"github.com/rs/zerolog/log"
+)
+
+func runL2CommandAndGetOutput(t *testing.T, cmdArgs ...string) string {


If this is a utility method (looks like it), we should not be taking in the testing object as a parameter. It confuses the reader.

In functions, as a rule, strive for minimum number of params.

For sophistication, compose functions

Okay got it, I have a similar function in anohter file will add both in test utils file

shrsv · 2023-08-16T14:40:06Z

Env/env.go

@@ -0,0 +1,48 @@
+package env


Recommend changing the package name to something more non-generic (env is a common name, with meaning at systems level).

Maybe call it: l2env

shrsv · 2023-08-16T14:40:50Z

Env/env.go

+		os.Exit(0)
+	}
+	// Check if it's an -e invocation
+	if o.Env != "UNSET_VU5TRVQ" {


As mentioned earlier, if it is o.Env we should stop processing, and exit the app.

In this function im exiting, should I remove it from this and use the exit in the conditions?
MarshalAndPrintJSON()

The name and logic doesn't match. What you are actually doing is:

MarshalAndPrintJSONAndExit :)

It is important to keep meaning and actions same.

Either name should be printExit or something like that, or you should move exit outside.

Right :|
Ive added in the conditions

shrsv · 2023-08-16T14:42:39Z

tests/utils/test_utils.go

+	return nil
+}
+
+func getL2BinaryPath() (string, error) {


getLocalL2BinaryPath to differentiate from system binary.

shrsv

LGTM 🍾

Feat/monaco

[feat]: Firecamp Echo Server

lovestaco added 5 commits August 12, 2023 15:32

Env suggesting with more revelance

d94e88b

Adding go-autocomplete-trie

Merge branch 'main' of github.com:HexmosTech/Lama2

768eb9f

Extracting common functionalities

a137b34

Adding tests to verify trie's functionalities, testing

a11245a

Adding more condition for empty env suggestion checking

5a0bd89

lovestaco self-assigned this Aug 13, 2023

Adding about relevant string, docuemntation

ca5fc33

shrsv reviewed Aug 14, 2023

View reviewed changes

lovestaco added 3 commits August 15, 2023 15:30

Adding default value to Environment variable

ccba626

Updating basic get test, testing

e1fab89

Adding basic command, testing

da128e0

rogueloop reviewed Aug 15, 2023

View reviewed changes

shrsv reviewed Aug 15, 2023

View reviewed changes

lovestaco added 4 commits August 16, 2023 10:04

Extracting env related functions, coderefactor

4e04ebb

Extracting utils related functions, coderefactor

5a0227c

Extracting test common functions, coderefactor

2b6e35e

Making compose functions and extracting repeated func, coderefactor

c9a2bc1

shrsv reviewed Aug 16, 2023

View reviewed changes

Renaming packeges, functions, coderefactor

6689711

shrsv approved these changes Aug 16, 2023

View reviewed changes

shrsv merged commit b526cf1 into main Aug 16, 2023
1 check passed

LinceMathew pushed a commit that referenced this pull request Dec 28, 2023

Merge pull request #32 from firecamp-io/feat/monaco

a5b2520

Feat/monaco

LinceMathew pushed a commit that referenced this pull request Dec 28, 2023

Merge pull request #32 from firecamp-dev/feat/echo-server

98aa2b5

[feat]: Firecamp Echo Server

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Origin/suggest relevant envs #32

Origin/suggest relevant envs #32

lovestaco commented Aug 13, 2023 •

edited

Loading

shrsv commented Aug 14, 2023

shrsv Aug 14, 2023

lovestaco Aug 15, 2023

rogueloop Aug 15, 2023

lovestaco Aug 16, 2023

shrsv commented Aug 15, 2023

shrsv Aug 15, 2023

lovestaco Aug 16, 2023

shrsv Aug 15, 2023

shrsv Aug 15, 2023

lovestaco Aug 16, 2023

shrsv Aug 15, 2023

lovestaco Aug 16, 2023

shrsv Aug 15, 2023

lovestaco Aug 16, 2023

shrsv Aug 15, 2023

lovestaco Aug 16, 2023

lovestaco Aug 16, 2023

shrsv Aug 15, 2023 •

edited

Loading

lovestaco Aug 16, 2023

shrsv Aug 16, 2023

shrsv Aug 16, 2023

lovestaco Aug 16, 2023

lovestaco Aug 16, 2023

shrsv Aug 16, 2023

lovestaco Aug 16, 2023

shrsv Aug 16, 2023

lovestaco Aug 16, 2023

shrsv left a comment

Origin/suggest relevant envs #32

Origin/suggest relevant envs #32

Conversation

lovestaco commented Aug 13, 2023 • edited Loading

What type of MR is this?

Description

Issue

Important files to start review from

l2 --help

Added tests?

Added to documentation?

If documentation update is there then run make mkdocs once PR is in acceptable state.

shrsv commented Aug 14, 2023

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

shrsv commented Aug 15, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrsv Aug 15, 2023 • 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

shrsv left a comment

Choose a reason for hiding this comment

lovestaco commented Aug 13, 2023 •

edited

Loading

If documentation update is there then run `make mkdocs` once PR is in acceptable state.

shrsv Aug 15, 2023 •

edited

Loading