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

Allow binding to environment variables #1859

Merged
merged 29 commits into from
May 18, 2023

Conversation

apedroferreira
Copy link
Member

@apedroferreira apedroferreira commented Apr 5, 2023

Closes #1785

Allow binding HTTP request headers to environment variables (or any other property besides request headers, if we enable it to).
Includes tests and documentation.

Screen.Recording.2023-05-17.at.19.07.14.mov

@apedroferreira apedroferreira added the new feature New feature or request label Apr 5, 2023
@apedroferreira apedroferreira self-assigned this Apr 5, 2023
@apedroferreira apedroferreira marked this pull request as ready for review April 6, 2023 12:36
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 12, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 14, 2023
@apedroferreira apedroferreira requested review from a team and Janpot April 14, 2023 14:03
@apedroferreira

This comment was marked as outdated.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 20, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 27, 2023
Copy link
Member

@bharatkashyap bharatkashyap left a comment

Choose a reason for hiding this comment

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

Looks good!

@@ -363,10 +363,10 @@ function QueryEditor({
() => ({
parameters: previewParams,
process: {
env,
env: Object.fromEntries(envVarNames.map((varName) => [varName, '<SECRET>'])),
Copy link
Member

@Janpot Janpot May 9, 2023

Choose a reason for hiding this comment

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

Oh, we're evaluating some bindings on the client side...
Then I guess it isn't possible to only send the variable names?
😕 This is not great

Copy link
Member Author

Choose a reason for hiding this comment

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

So it's not secure this way? I thought it only uses the real environment values if it runs server-side :/ Shouldn't that be possible?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yes client-side it won't use the real values in this case, yeah...

Copy link
Member Author

Choose a reason for hiding this comment

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

I can show <SECRET> and use the real values in the client anyway if that's still safe.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can do either one of those:

  • Send the whole env like you did before and let it be
  • Rethink how to use secrets in rest datasource and instead offer a new type of binding, next to javascript expression, allow binding directly to an environment variable through a dropdown

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel like we can go with the first option "Send the whole env like you did before and let it be".
But let me know if you think the second one is better for any reason and it would not be that much extra work anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Some arguments I can think of that would be in favor for the second solution:

  • It's easier for us to detect which environment variables an app expects to be run under. That allows us to warn on any missing ones, e.g. when a user checks out an app for the first time. It could somewhat be extracted from the javascript bindings by regex, or static analysis, but it wouldn't be a foolproof solution.
  • We could more fine grained specify where exactly we want this to be used. e.g. only in authentication/headers
  • Thinking long term, suppose we decide that whatever we came up with in this PR was a mistake, we can migrate from $env: MY_VAR to $jsExpression: 'process.env.MY_var' and guarantee no breaking change, we can't do that on the inverse.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair points, ok I'll change it to be a new type of binding, having it as $env sounds consistent with the other types.

Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

Looking good. I'd change a few things:

  1. avoid the layout shift when switching tabs in the binding editor

    Screen.Recording.2023-05-14.at.12.11.39.mov
  2. Make it a free input with autocomplete. We can give an indication when we don't find the environment variable in the env file, but I feel like we should allow input. As a follow up we can prompt the user for a value if we don't find it in the env file and add it automatically

  3. Update the rest-basic test with some checks for environment variables.

Additionally we should update the HTTP query docs. This can be done separately as well if you want

@@ -301,6 +308,7 @@ export const META = {
definitions: {
Json: jsonSchema,
JsExpressionBinding: jsExpressionBindingSchema,
EnvBinding: envBindingSchema,
Copy link
Member

@Janpot Janpot May 13, 2023

Choose a reason for hiding this comment

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

Just a note, but after beta we will have to think about a migration strategy, because this is not backwards compatible

Copy link
Member Author

Choose a reason for hiding this comment

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

But it's not a breaking change, just a new type of binding, right? Why would it break with previous versions?

Copy link
Member

Choose a reason for hiding this comment

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

An app created with this schema won't work in older Toolpad versions. Older Toolpad versions don't know what $env means

Copy link
Member Author

@apedroferreira apedroferreira May 15, 2023

Choose a reason for hiding this comment

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

Oh, I thought we only needed to make to make old schemas be able to migrate to newer versions...
If we have backwards compatibility like this I guess we will need to migrate, yep.

Copy link
Member

Choose a reason for hiding this comment

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

The general idea would be:

  • old schema + new toolpad => we should be able to internally migrate and display the app + give option for user to migrate files to new version, or do it automatically
  • new schema + old toolpad => either all features of the schema are supported in Toolpad, or otherwise we need to tell the user "This app as created in newer Toolpad, please upgrade Toolpad to version X if you want to run/edit this application"

Copy link
Member Author

@apedroferreira apedroferreira May 16, 2023

Choose a reason for hiding this comment

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

At first thought the 2nd option (showing the user a message) seems more sane to me and not that unusual?
But we can try the first one if it's not that hard, also depending on how common it would be for users to try to use new schemas in old Toolpad versions.

Copy link
Member

@Janpot Janpot May 16, 2023

Choose a reason for hiding this comment

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

also depending on how common it would be for users to try to use new schemas in old Toolpad versions.

This scenario could happen when people copy toolpad apps from one repo to another

packages/toolpad-app/src/toolpadDataSources/rest/server.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 15, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 16, 2023
@apedroferreira

This comment was marked as resolved.

@Janpot

This comment was marked as resolved.

@apedroferreira apedroferreira changed the title Ability to access process in HTTP query bindings Allow binding to environment variables May 17, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 17, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 17, 2023
Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

this is great!

@apedroferreira apedroferreira merged commit f257e90 into master May 18, 2023
@apedroferreira apedroferreira deleted the use-process-env-in-http-queries branch May 18, 2023 15:05
@prakhargupta1
Copy link
Member

This should be very helpful! I'll resume work on some of the public apps that needed this feature.
cc: @oliviertassinari

@oliviertassinari
Copy link
Member

oliviertassinari commented May 22, 2023

Ah, yes, awesome, we can move more apps https://github.com/mui/mui-private/issues/267. It's different than #1952 but would still solve the problem.

@Janpot Janpot mentioned this pull request Jun 1, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

For an authenticated REST API, allowed editing from the fetch editor
5 participants