feat: add env variables in start session form #2066

andre-code · 2022-10-10T19:00:19Z

This PR add the env variables fields in start session form.

Testing

User should be able to start a session with environment variables
Verify environment variable is available
Create a session link to share with environment variables

Closes #2058

*Note: Added renku-notebooks dependency just to make available other functionalities merged before in master
/deploy #persist renku-notebooks=master extra-values=notebooks.amalthea.image.repository=registry.dev.renku.ch/tasko.olevski/renku-images/amalthea,notebooks.amalthea.image.tag=0.5.2-n014.h50ddcaa

RenkuBot · 2022-10-10T19:01:30Z

You can access the deployment of this PR at https://renku-ci-ui-2066.dev.renku.ch

lorenzo-cavazzi

Nice addition! It works well, but I feel the logic for storing the environment variables in the presentation component is redundant since you already store them in the redux store.

I would remove const [environmentVariables, setEnvironmentVariables] = useState( and use the variables coming from the redux model direcly.

lorenzo-cavazzi · 2022-10-12T08:56:46Z

client/src/model/RenkuModels.js

@@ -391,6 +391,7 @@ const notebooksSchema = new Schema({
      commit: { initial: {} },
      discard: { initial: false },
      options: { initial: {} },
+      environment_variables: { initial: [{ key: "", value: "" }] },


Suggested change

environment_variables: { initial: [{ key: "", value: "" }] },

environment_variables: { initial: {} },

If I interpret the code correctly, this format [{ key: "", value: "" }] is overwritten by { <key>: <value> } immediately.

P.S: after finishing the review, I now understand this better.
I feel going for [{ key: "", value: "" }] is the best alternative since it's easier to add empty fields.

lorenzo-cavazzi · 2022-10-12T09:03:38Z

client/src/notebooks/NotebookStart.present.js

+  const [environmentVariables, setEnvironmentVariables] = useState([ { key: "", value: "" }]);
+
+  const setNotebookEnvVariables = (variables) => {
+    props.handlers.setNotebookEnvVariables(variables);
+    setEnvironmentVariables(variables);
+  };
+
+  useEffect(() => {
+    if (props.envVariablesQueryParams)
+      setEnvironmentVariables(props.envVariablesQueryParams);
+  }, []); // eslint-disable-line
+


This logic seems redundant to me.
If you are already save the status in the redux store, you can get it from props.filters.environment_variables.
Why saving it locally with a different format? Wouldn't it be easier to use only the redux store?

I feel the easiest thing would be adopting the pattern [ { key: "", value: "" }] also for the redux store, although that requires a few extra changes.

andre-code · 2022-10-12T11:36:19Z

Nice addition! It works well, but I feel the logic for storing the environment variables in the presentation component is redundant since you already store them in the redux store.

I would remove const [environmentVariables, setEnvironmentVariables] = useState( and use the variables coming from the redux model direcly.

Thanks @lorenzo-cavazzi for your review.
I agree to use the redux state in the environmentVariables component, that would be cleaner and remove some of the complexity.
Find the suggestion implemented in the last commit.

lorenzo-cavazzi

This looks great! A tiny detail before merging.

I personally think we can start with an empty array for the variables and let the user click on "Add Variable" instead of showing an empty one since it's optional and probably only very few people will use them. The initial state would be as in the following screenshot.

If you agree, I think I added all the necessary changes as suggestions. If you prefer to keep an initial empty entry, I would then move const DEFAULT_ENV_VARIABLES away from the RenkuModel file to a "NotebooksSomething" file since it looks like a session-related constant.

lorenzo-cavazzi · 2022-10-12T13:15:43Z

client/src/model/RenkuModels.js

+const DEFAULT_ENV_VARIABLES = [{ key: "", value: "" }];
+


Suggested change

const DEFAULT_ENV_VARIABLES = [{ key: "", value: "" }];

lorenzo-cavazzi · 2022-10-12T13:15:56Z

client/src/model/RenkuModels.js

@@ -391,6 +393,7 @@ const notebooksSchema = new Schema({
      commit: { initial: {} },
      discard: { initial: false },
      options: { initial: {} },
+      environment_variables: { initial: DEFAULT_ENV_VARIABLES },


Suggested change

environment_variables: { initial: DEFAULT_ENV_VARIABLES },

environment_variables: { initial: [] },

lorenzo-cavazzi · 2022-10-12T13:16:06Z

client/src/model/RenkuModels.js

@@ -722,5 +725,5 @@ const formGeneratorSchema = new Schema({
 export {
  datasetFormSchema, datasetSchema, datasetImportFormSchema, environmentSchema,
  formGeneratorSchema, issueFormSchema, newProjectSchema, notebooksSchema, notificationsSchema,
-  projectSchema, projectsSchema, statuspageSchema, userSchema, webSocketSchema
+  projectSchema, projectsSchema, statuspageSchema, userSchema, webSocketSchema, DEFAULT_ENV_VARIABLES


Suggested change

projectSchema, projectsSchema, statuspageSchema, userSchema, webSocketSchema, DEFAULT_ENV_VARIABLES

projectSchema, projectsSchema, statuspageSchema, userSchema, webSocketSchema

lorenzo-cavazzi · 2022-10-12T13:16:21Z

client/src/notebooks/Notebooks.container.js

@@ -30,6 +30,7 @@ import { StatusHelper } from "../model/Model";
 import { Url } from "../utils/helpers/url";
 import { sleep } from "../utils/helpers/HelperFunctions";
 import ShowSessionFullscreen from "./components/SessionFullScreen";
+import { DEFAULT_ENV_VARIABLES } from "../model/RenkuModels";


Suggested change

import { DEFAULT_ENV_VARIABLES } from "../model/RenkuModels";

lorenzo-cavazzi · 2022-10-12T13:16:36Z

client/src/notebooks/Notebooks.container.js

+
+    const environmentVariables = this.getEnvironmentVariablesFromSearch(currentSearch);
+    this.customEnvVariables = currentSearch && this.autostart && environmentVariables.length ?
+      environmentVariables : DEFAULT_ENV_VARIABLES;


Suggested change

environmentVariables : DEFAULT_ENV_VARIABLES;

environmentVariables : [];

lorenzo-cavazzi

Lgtm! 🚀

RenkuBot · 2022-10-14T17:44:07Z

Tearing down the temporary RenkuLab deplyoment for this PR.

andre-code temporarily deployed to renku-ci-ui-2066 October 10, 2022 19:00 Inactive

andre-code force-pushed the 2058-env-variables-in-share-link branch from dc32fca to 88db8e8 Compare October 11, 2022 10:58

andre-code temporarily deployed to renku-ci-ui-2066 October 11, 2022 10:58 Inactive

andre-code temporarily deployed to renku-ci-ui-2066 October 11, 2022 13:34 Inactive

andre-code force-pushed the 2058-env-variables-in-share-link branch from 88db8e8 to d98c125 Compare October 12, 2022 07:00

andre-code temporarily deployed to renku-ci-ui-2066 October 12, 2022 07:01 Inactive

andre-code marked this pull request as ready for review October 12, 2022 07:31

andre-code requested a review from a team as a code owner October 12, 2022 07:31

andre-code temporarily deployed to renku-ci-ui-2066 October 12, 2022 07:31 Inactive

andre-code force-pushed the 2058-env-variables-in-share-link branch from d98c125 to bee2968 Compare October 12, 2022 09:05

andre-code temporarily deployed to renku-ci-ui-2066 October 12, 2022 09:05 Inactive

lorenzo-cavazzi requested changes Oct 12, 2022

View reviewed changes

andre-code temporarily deployed to renku-ci-ui-2066 October 12, 2022 10:00 Inactive

andre-code force-pushed the 2058-env-variables-in-share-link branch from d4d6375 to 217f096 Compare October 12, 2022 10:23

andre-code temporarily deployed to renku-ci-ui-2066 October 12, 2022 10:23 Inactive

andre-code had a problem deploying to renku-ci-ui-2066 October 12, 2022 12:10 Error

andre-code temporarily deployed to renku-ci-ui-2066 October 12, 2022 12:15 Inactive

lorenzo-cavazzi requested changes Oct 12, 2022

View reviewed changes

andre-code temporarily deployed to renku-ci-ui-2066 October 12, 2022 14:24 Inactive

andre-code temporarily deployed to renku-ci-ui-2066 October 12, 2022 14:34 Inactive

andre-code added 3 commits October 13, 2022 10:54

feat: add env variables in start session form

1448dbb

refactor: use redux environment_variables for EnvironmentVariables state

53b93de

feat: no show empty inputs as defaults

dfb4105

andre-code force-pushed the 2058-env-variables-in-share-link branch from 3469fb1 to dfb4105 Compare October 13, 2022 08:54

andre-code temporarily deployed to renku-ci-ui-2066 October 13, 2022 08:54 Inactive

andre-code temporarily deployed to renku-ci-ui-2066 October 13, 2022 14:12 Inactive

andre-code force-pushed the 2058-env-variables-in-share-link branch from df56e0d to f08b01c Compare October 13, 2022 15:08

andre-code had a problem deploying to renku-ci-ui-2066 October 13, 2022 15:08 Failure

andre-code force-pushed the 2058-env-variables-in-share-link branch from f08b01c to 5470841 Compare October 14, 2022 06:57

andre-code temporarily deployed to renku-ci-ui-2066 October 14, 2022 06:58 Inactive

andre-code force-pushed the 2058-env-variables-in-share-link branch from 5470841 to 9e232af Compare October 14, 2022 08:03

andre-code temporarily deployed to renku-ci-ui-2066 October 14, 2022 08:03 Inactive

andre-code temporarily deployed to renku-ci-ui-2066 October 14, 2022 08:18 Inactive

lorenzo-cavazzi previously approved these changes Oct 14, 2022

View reviewed changes

fix: read multiple param variables with the same key

414e2d6

andre-code dismissed lorenzo-cavazzi’s stale review via 414e2d6 October 14, 2022 09:29

andre-code force-pushed the 2058-env-variables-in-share-link branch from baf48aa to 414e2d6 Compare October 14, 2022 09:29

andre-code temporarily deployed to renku-ci-ui-2066 October 14, 2022 09:29 Inactive

Merge branch 'master' into 2058-env-variables-in-share-link

eb04a24

andre-code had a problem deploying to renku-ci-ui-2066 October 14, 2022 13:31 Failure

andre-code deployed to renku-ci-ui-2066 October 14, 2022 13:34 Active

lorenzo-cavazzi approved these changes Oct 14, 2022

View reviewed changes

andre-code merged commit 7c6e24f into master Oct 14, 2022

andre-code deleted the 2058-env-variables-in-share-link branch October 14, 2022 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add env variables in start session form #2066

feat: add env variables in start session form #2066

andre-code commented Oct 10, 2022 •

edited

Loading

RenkuBot commented Oct 10, 2022

lorenzo-cavazzi left a comment

lorenzo-cavazzi Oct 12, 2022

lorenzo-cavazzi Oct 12, 2022

lorenzo-cavazzi Oct 12, 2022

andre-code commented Oct 12, 2022

lorenzo-cavazzi left a comment

lorenzo-cavazzi Oct 12, 2022

lorenzo-cavazzi Oct 12, 2022

lorenzo-cavazzi Oct 12, 2022

lorenzo-cavazzi Oct 12, 2022

lorenzo-cavazzi Oct 12, 2022

lorenzo-cavazzi left a comment

RenkuBot commented Oct 14, 2022

	environment_variables: { initial: [{ key: "", value: "" }] },
	environment_variables: { initial: {} },

	environment_variables: { initial: DEFAULT_ENV_VARIABLES },
	environment_variables: { initial: [] },

	projectSchema, projectsSchema, statuspageSchema, userSchema, webSocketSchema, DEFAULT_ENV_VARIABLES
	projectSchema, projectsSchema, statuspageSchema, userSchema, webSocketSchema

	environmentVariables : DEFAULT_ENV_VARIABLES;
	environmentVariables : [];

feat: add env variables in start session form #2066

feat: add env variables in start session form #2066

Conversation

andre-code commented Oct 10, 2022 • edited Loading

Testing

RenkuBot commented Oct 10, 2022

lorenzo-cavazzi 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

andre-code commented Oct 12, 2022

lorenzo-cavazzi 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

lorenzo-cavazzi left a comment

Choose a reason for hiding this comment

RenkuBot commented Oct 14, 2022

andre-code commented Oct 10, 2022 •

edited

Loading