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

Added nested keys check after root keys check for environment editor. #2685

Merged
merged 18 commits into from
Sep 10, 2021

Conversation

johnhorsema
Copy link
Contributor

Closes #2684

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Nice first attempt 👏

After #2601, keys themselves can contain spaces, so after calling getNestedKeys, splitting on a single space won't quite work as intended. Additionally, getNestedKeys will iterate over all keys first, and then check each, one by one. This needs to be made much more efficient in that as soon as a key that does not follow the regex rule is detected, the algorithm stops.

Remember, the example linked in the issue is an example, from which a new solution needs to be derived for the environment-editor specifically. 😄 Let me know if you'd like further direction here, but I'll let you improve the approach with this feedback in mind.

Lastly, please add some screenshots for what the user will see when adding a nested key which does not follow the rules, as well as some unit tests in environment-editor.test.js.

@johnhorsema
Copy link
Contributor Author

Lastly, please add some screenshots for what the user will see when adding a nested key which does not follow the rules, as well as some unit tests in environment-editor.test.js.

Screenshots
001
002
003

@johnhorsema
Copy link
Contributor Author

@develohpanda please have a look at my latest PR, thanks

@develohpanda develohpanda self-requested a review October 5, 2020 04:36
Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Making some progress 👏 a couple of comments, particularly with the efficiency and clarity of the recursive function.

@johnhorsema
Copy link
Contributor Author

@develohpanda please have a look at my latest PR, thanks

@develohpanda
Copy link
Contributor

@johnhorsema Thanks! Don't worry, I haven't missed it, just been quite busy. I will re-review when I get a chance to. 🤗

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Sorry about the delay, I finally had a chance to review this. I'm generally happy with it, did some testing locally and it seems to be working nicely.

I've added a couple of comments that are worth addressing, please! 🙂

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@vercel vercel bot temporarily deployed to Preview September 7, 2021 00:59 Inactive
Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

QA'd with the following environment (and by changing it round): https://www.loom.com/share/1fa103f7ce0141c286da62dad146e147.

@gatzjames @dimitropoulos the rules for root and nested keys are defined in #2684.

{
  "base-url": "https://api.insomnia.rest",
  "nested": {
    "path-with-hyphens": "/path-with-hyphen"
  },
  "ar-ray": [
    "/first",
    {
      "second": "second"
    },
    {
      "third": "third"
    }
  ]
}

Thanks for working through this, apologies for the mega delays; we're working through the older PRs.

Copy link
Contributor

@dimitropoulos dimitropoulos left a comment

Choose a reason for hiding this comment

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

works great. I really appreciate all the tests, too!

@dimitropoulos dimitropoulos enabled auto-merge (squash) September 10, 2021 19:05
@dimitropoulos dimitropoulos merged commit 7e7d235 into Kong:develop Sep 10, 2021
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.

Validate environment variables against nested keys
5 participants