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

Global variable support and Generate env JSON option #24

Merged
merged 23 commits into from
Jul 20, 2023

Conversation

lovestaco
Copy link
Contributor

@lovestaco lovestaco commented Jul 10, 2023

What type of MR is this?

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Doc Update
  • 🎨 Style
  • πŸ§‘β€πŸ’» Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • ⏩ Revert

Description

This MR adds the feature of supporting global variables.
Edit1:

  • L2 can now fetch variables declared in l2config.env which can be in the parent directory.

  • l2config.env is searched for, from the present directory to all its ancestors (upto /).

  • A directory may have both l2.env AND l2config.env

Edit2:

  • Added a new option for l2command l2 --env <l2FilePath>.
  • L2 will output a JSON of environment variables with metadata from l2config.env and l2.env.
  • Added this feature so frontend i.e. IntelliJ/ VScode suggesting variables can be done in a quicker and more detailed high-quality way.
{
  "KARMA_LOCAL_URL": {
    "src": "l2configenv",
    "val": "http://127.0.0.1:8000"
  },
  "PARSE_URL_LOCAL": {
    "src": "l2env",
    "val": "http://localhost:1337/parse"
  }
}

Important code file to start Code Review from

controller.go

Added tests?

  • πŸ‘ yes
  • πŸ™… no, because they aren't needed
  • πŸ™‹ no, because I need help

Added to documentation?

  • πŸ““ l2format.md
  • πŸ““ architecture.md
  • πŸ“œ preprocess.md
  • πŸ“œ examples.md

@shrsv
Copy link
Contributor

shrsv commented Jul 10, 2023

After every documentation change - we are supposed to run the following and commit all resultant files:

mkdocs:
	cd docs/Lama2; ./build.sh

Anyway - wait till my overall review finishes, then you can do this.

@@ -69,8 +69,18 @@ Get [Source File](https://github.com/HexmosTech/Lama2/tree/main/examples/0003_co

## Environment Variables: Switch base URL

Copy link
Contributor

@shrsv shrsv Jul 10, 2023

Choose a reason for hiding this comment

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

We can make the documentation more structured (something like this)

Case 1: l2.env adjacent to an API file

For any given .l2 file, one can place an l2.env file to store relevant variables. These variables will be available to be used within the API file

[example]

Case 2: Project-level variables

In Lama2, you can have a large number of API files stored in a hierarchical folder configuration. The root of such a project can be signified through l2config.env:

[API Hub folder screenshot with l2config.env highlighted]

Within such a structure, you can have an API file anywhere, which can use variables defined in the project-level variables:

[Example l2config.env and api.l2 with appropriate folder structure; the example must be hosted in Lama2 repo (see other examples)]

Case 3: Override Project-level variable with local variable

....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it

@@ -67,7 +67,7 @@ func GetLamaFileAsString(path string) string
func LamaFile(inputFile string) (string, string)
```

LamaFile takes in a path to an API file. It moves into the API file directory, reads the API contents, loads the \`l2.env\` file if available, and finally substitutes environment vars in the API contents Once done, it reverts back to the original directory, and returns the processed l2 file.
LamaFile takes in a path to an API file. It moves into the API file directory, reads the API contents, loads the \`l2config.env\` global variables file if available in any of parent directory and \`l2.env\` file if available in present directory , and finally substitutes environment vars in the API contents Once done, it reverts back to the original directory, and returns the processed l2 file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment can make the "overwriting" part more obvious. Sentence is too long now, maybe simplify a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@@ -48,7 +48,7 @@ From a high level, how does it work now?
2. Else if block is Requestor block
1. Replace variables with values in the following order
1. Try fetch variable from Javascript VM
2. If (1) fails, try fetch variable from `l2.env`
2. If (1) fails, try fetch variable from `l2config.env` and `l2.env`
Copy link
Contributor

Choose a reason for hiding this comment

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

"overwriting" is an implementation detail. When you talk to users - how does it work?

  1. JS VM variable
  2. Local env variable
  3. l2config env variable.

Always explain in this way. The implementation detail doesn't matter at documentation/explanation level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay got it

@@ -83,6 +83,17 @@ Cookies are specified in a `Cookie` header as follows:
Cookie:'sessionid=foo;another-cookie=bar'
```

### Global Environments variables/commands can be defined in `/l2config.env/<requests_dir>`

The *l2* loads up variables from `l2config.env` and then `l2.env`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Again - the overwrite explanation is more complex to the reader/user. Simply say what order in which variables are searched for (nearest first):

  1. VM
  2. local
  3. global

Copy link
Contributor

Choose a reason for hiding this comment

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

Either:

The l2 binary loads...

or

l2 loads

@@ -83,6 +83,17 @@ Cookies are specified in a `Cookie` header as follows:
Cookie:'sessionid=foo;another-cookie=bar'
```

### Global Environments variables/commands can be defined in `/l2config.env/<requests_dir>`
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of calling it "global", I think we should call it something like:

  1. Project-level variables
  2. Root variables
  3. Project-root variables

Let's pick one of the above and finalize (or make other suggestions).

Second, I think we should use git repo analogy to explain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added the git repo analogy in example.md, should I link it here as well?

Copy link
Contributor

@shrsv shrsv Jul 12, 2023

Choose a reason for hiding this comment

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

Analogy should be in the explanation section, not example section. When confused what belongs where, re-read this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@@ -83,6 +83,17 @@ Cookies are specified in a `Cookie` header as follows:
Cookie:'sessionid=foo;another-cookie=bar'
```

### Global Environments variables/commands can be defined in `/l2config.env/<requests_dir>`
Copy link
Contributor

Choose a reason for hiding this comment

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

Global Environments variables/commands can be defined in /l2config.env/<requests_dir>

Title is a bit clumsy.

API environment variables can be defined at project level using l2config.env

Is much simpler.

@shrsv
Copy link
Contributor

shrsv commented Jul 10, 2023

Remaining comments - I think we still do not have functionality for obtaining the variable suggestions from VSCode (using some CLI switch - pls make proposals). That can be added to this MR itself.

@shrsv
Copy link
Contributor

shrsv commented Jul 10, 2023

In general - nice first MR πŸ‘

@shrsv
Copy link
Contributor

shrsv commented Jul 10, 2023

Also - we must add test case(s) for this new logic.

- Command to get the envs `l2 --env ${l2FilePath}`.
- All the envs of l2config.env is fetched and overwritten by l2.env.
- Creates a JSON of all the envs with its value and source of origin.
@lovestaco lovestaco changed the title Global variable support for Lama2 Global variable support and Generate env JSON option Jul 16, 2023
@shrsv
Copy link
Contributor

shrsv commented Jul 16, 2023

L2 can now fetch variables declared in l2config.env which can be in the parent directory.

L2 will first fetch the variables from the l2config.env which can be in the parent directory and
then fetch the variables from l2.env from the present dir.

The description has to be more precise.

  1. l2config.env is searched for, from the present directory to all its ancestors (upto /). Yes, a directory may have both l2.env AND l2config.env (make sure this is so)


### API environment variables can be defined at project level using `l2config.env`

_l2_ loads local variables from `l2.env`.
Copy link
Contributor

@shrsv shrsv Jul 16, 2023

Choose a reason for hiding this comment

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

This is confusing to the end user. You mean - at file level or at variable?

The fact is, we load variables from both files. If there is ever a conflict in a particular variable name, the way prioritize selection is:

  1. VM vars
  2. l2.env vars
  3. l2config.env vars

_l2_ loads local variables from `l2.env`.
If not found then uses variables from `l2config.env`.
Example `l2config.env`:

Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot needed to show project structure, and root.

### If API environment variables are locally and at project level

The local variable's value is taken into consideration

Copy link
Contributor

Choose a reason for hiding this comment

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

Must show full example here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@@ -96,11 +99,85 @@ func ExpandJSON(block *gabs.Container, vm *goja.Runtime) {
log.Debug().Str("Processed JSON block", block.String()).Msg("")
}

func LoadElfEnv(l2path string) {
func SearchL2ConfigEnv(dir string) (string, error) {
parentDir := filepath.Dir(dir)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should start from present folder itself. What if it is a project without any subdirs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this case failed :(

return
}
defer file.Close()
return godotenv.Parse(file)
Copy link
Contributor

@shrsv shrsv Jul 16, 2023

Choose a reason for hiding this comment

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

One thing to verify is - does this properly handle back-tick commands?

Say I have an echo statement or something, is it going to do the right thing?

Are our test cases checking this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like this in the l2.env file ?

export ECHO_CMD="`echo "base64 image.jpeg"`"

Copy link
Contributor Author

@lovestaco lovestaco Jul 18, 2023

Choose a reason for hiding this comment

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

export PHOTO=`base64 image.jpeg`

?

Copy link
Contributor

@shrsv shrsv Jul 18, 2023

Choose a reason for hiding this comment

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

yes, the photo case must work (check tests/examples).

in general, the backticks can have any arbitrary linux command - which runs first, and then the output is stored in the env variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No there is no test on that, will add

return err
}
for key, value := range envs {
variable := make(map[string]interface{})
Copy link
Contributor

Choose a reason for hiding this comment

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

8 spaces? use the gofumpt or some other thing in the makefile to format.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can run both of these and make any mods:


lint:
	golangci-lint run --disable-all -E revive ./...

gofumpt:
	gofumpt -w .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

if err != nil {
log.Error().Str("Type", "Preprocess").Msg(err.Error())
} else {
err = populateEnvMap(finalEnvMap, l2ConfigPath, "l2configenv")
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is confusing. Try to write a pure functions:

  1. explicit inputs
  2. no modification of inputs
  3. clear outputs (new variables)
  4. reassign outputs here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes removed the finalMap modifying part.

@lovestaco lovestaco self-assigned this Jul 16, 2023
log.Info().Str("Type", "Preprocess").Msg("Env Variables:\n" + string(jsonEnvs))
return
// Frontend can read the stdout for this command and get the JSON of all the env's
fmt.Println(string(jsonEnvs))
Copy link
Contributor

Choose a reason for hiding this comment

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

Just check that with verbose mode as well - this works. I think we have a -v and -vv option IIRC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes works.
image


The local variable's value is taken into consideration
The local variable's value is taken into consideration regardless of both files reside in same directory
Copy link
Contributor

Choose a reason for hiding this comment

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

*residing

@@ -114,8 +114,8 @@ func getShellName(s string) (string, int) {
}
// Scan alphanumerics.
var i int
for i = 0; i < len(s) && isAlphaNum(s[i]); i++ {
}
// for i = 0; i < len(s) && isAlphaNum(s[i]); i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why comment this? If lines not needed, should be deleted.

return envMap, nil
}

func combineEnvMaps(envMaps ...map[string]map[string]interface{}) map[string]map[string]interface{} {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment regarding desired order for l2config.env and l2.env in this case. What overwrites what?

@shrsv
Copy link
Contributor

shrsv commented Jul 18, 2023

Also - we must add test case(s) for this new logic.

This has not been addressed @lovestaco

- Making changes in the Documentation.
@lovestaco
Copy link
Contributor Author

Also - we must add test case(s) for this new logic.

This has not been addressed @lovestaco

https://github.com/HexmosTech/Lama2/blob/globalvar-l2configenv/elfparser/ElfTestSuite/root_variable_override

@@ -25,7 +25,7 @@ func (p *Lama2Parser) PrimitiveType() (*gabs.Container, error) {

// CustomPairMerge uses a gabs feature to deal with merge conflicts.
// More here: https://github.com/HexmosTech/gabs/blob/master/gabs.go#L511
func CustomPairMerge(destination, source interface{}) interface{} {
func CustomPairMerge(_, source interface{}) interface{} {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why rename this in jsonparser?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was erroring out in the make lint

Copy link
Contributor

@shrsv shrsv left a comment

Choose a reason for hiding this comment

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

LGTM πŸ₯³

@shrsv shrsv merged commit 33858b3 into main Jul 20, 2023
1 check passed
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.

2 participants