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

Environment Variable Template Import #2372

Closed
4 tasks
MarianRaphael opened this issue Jun 30, 2023 · 11 comments · Fixed by #2501
Closed
4 tasks

Environment Variable Template Import #2372

MarianRaphael opened this issue Jun 30, 2023 · 11 comments · Fixed by #2501
Assignees
Labels
size:M - 3 Sizing estimation point story A user-oriented description of a feature
Milestone

Comments

@MarianRaphael
Copy link
Contributor

MarianRaphael commented Jun 30, 2023

Description

As a Node-RED developer and FlowForge user,
I want to directly import environment variable templates,
So that I can minimize manual work and human error during the maintenance and input of environment variables.

Which customers would this be available to

All Users (CE)

Acceptance Criteria

  • an 'Edit Mode' where I can upload a complete environment variable template at once rather than having to input each field individually
  • verify the pasted data to ensure it's in the correct format before attempting to import, showing an error message if the format is incorrect
  • support classic formats like .env
  • API support

Have you provided an initial effort estimate for this issue?

I have provided an initial effort estimate

@MarianRaphael MarianRaphael added story A user-oriented description of a feature size:M - 3 Sizing estimation point labels Jun 30, 2023
@joepavitt
Copy link
Contributor

joepavitt commented Jun 30, 2023

Quick idea on changes to the env var page for how we could incorporate these:

Screen.Recording.2023-06-30.at.11.03.24.mov

Key changes:

  • Import .env file as per Portainer.
  • Hide the row for adding new environment variables by default, if they have many as implied, we also need to move this to the top (currently at the bottom of the table) to prevent excessive scrolling
  • We'd add confirmation/validation of values as part of the Modal/Dialog after the file has uploaded - didn't prototype this part.

@sumitshinde-84
Copy link
Contributor

sumitshinde-84 commented Jun 30, 2023

Can I work on this? @joepavitt @MarianRaphael

@joepavitt
Copy link
Contributor

Can I work on this?

We're actually going to have a pre-cursor to this which you're welcome to help out on: FlowFuse/forge-ui-components#56

We have a collection of custom-made widgets (forge-ui-components) and are missing a file upload widget as per the design above. If you're okay to add one, that'd be great.

@joepavitt
Copy link
Contributor

joepavitt commented Jun 30, 2023

Hide the row for adding new environment variables by default, if they have many as implied, we also need to move this to the top (currently at the bottom of the table) to prevent excessive scrolling

Actually, thinking about this further, we have no option to edit/delete env vars at the moment. Changing this to an "Edit Variables" button which toggles in text field inputs for the values and adds delete options too. @knolleary is there API functionality for that already, or would that increase scope to API changes too?

@knolleary
Copy link
Member

@joepavitt Users can edit/delete env vars in their instance settings. There are the set of platform-defined ones (FF_xxx) that cannot be edited/deleted as they are always present - but anything the user has added can be edited/deleted.

@joepavitt
Copy link
Contributor

Users can edit/delete env vars in their instance settings

🤦 right you are Nick, only had the FF_ examples in my view.

@MarianRaphael MarianRaphael modified the milestones: 1.11, 1.10 Jul 4, 2023
@Steve-Mcl Steve-Mcl self-assigned this Jul 17, 2023
@Steve-Mcl
Copy link
Contributor

Steve-Mcl commented Jul 17, 2023

@MarianRaphael @knolleary

Q1: It is common for .env files to contain helpful comments e.g:

# general
NODE_ENV=development

# promotions database
DB1_USERNAME=X
DB1_PASSWORD=X
DB1_HOST=X

# stock database
DB2_USERNAME=X
DB2_PASSWORD=X
DB2_HOST=X

# Stripe API key
PK=XYZ

Would we wish to maintain this format for the user? I can certainly see a benefit of free text entry / free text edit (for ease and speed of editing and for the built in comments) but this would remove the custom elements and replace it with simpler text area.


Q2: Are there any objections to adding the dotenv package? I would only be using the parse function (not loading or touching process.env) to parse and verify the .env data before committing it to database. It is my preference to include dotenv over a DIY solution. (dotenv has 35 million weekly downloads)


Q3: Multiline env vars are common and supported in dotenv files. e.g:

PRIVATE_KEY="-----BEGIN RSA PRIVATE KEY-----
...
Kh9NV...
...
-----END RSA PRIVATE KEY-----"

Do we

  1. support and maintain line breaks (would need modification to the custom control if we are switching to free form text)
  2. support but collapse them. E.g: PRIVATE_KEY="-----BEGIN RSA PRIVATE KEY-----\nKh9NV...\n-----END RSA PRIVATE KEY-----\n"
  3. other?

Q4: dotenv can support variable expansion. This can be added now or later (or never) but is a simple and very nice feature.

  • Include in this work?
  • Defer to future feature?
  • forget about it?

NOTE: Indicate in support (or no support) in docs to clarify the stance.

FYI (and to teach the sucking of eggs), variable expansion permits this kind of thing:

MONGOLAB_DATABASE=heroku_db
MONGOLAB_USER=username
MONGOLAB_PASSWORD=password
MONGOLAB_DOMAIN=abcd1234.mongolab.com
MONGOLAB_PORT=12345

MONGOLAB_USER_RECURSIVELY=${MONGOLAB_USER}:${MONGOLAB_PASSWORD}
MONGOLAB_URI_RECURSIVELY=mongodb://${MONGOLAB_USER_RECURSIVELY}@${MONGOLAB_DOMAIN}:${MONGOLAB_PORT}/${MONGOLAB_DATABASE}

MONGOLAB_URI=mongodb://$MONGOLAB_USER:$MONGOLAB_PASSWORD@$MONGOLAB_DOMAIN:$MONGOLAB_PORT/$MONGOLAB_DATABASE

MONGOLAB_USERPASS=$MONGOLAB_USER:$MONGOLAB_PASSWORD
MONGOLAB_URI2=mongodb://$MONGOLAB_USERPASS@$MONGOLAB_DOMAIN:$MONGOLAB_PORT/$MONGOLAB_DATABASE

@knolleary
Copy link
Member

Q1: The requirement is to provide a simple way to import en masse a set of env vars. The imported file can contain comments, but those comments don't get saved anywhere - we store env vars as key/value pairs.

Q2: Sure

Q3: Our env var UI doesn't support multi-line env vars as they are presented as <input> fields that will strip any new lines. Please raise a separate issue for adding support for multi-line env var value entry - it is not in scope for this issue.

Q4: variable expansion is out of scope for this item. We haven't had any requests to support it.

@Steve-Mcl
Copy link
Contributor

The requirement is to provide a simple way to import en masse a set of env vars.

@knolleary forgot to ask what the merge rules are to be (I have scanned the issue but dont see it in the acceptance criteria or comments).

Q1: When an env var upload is performed,

  • Do we delete all existing Env Vars and use the imported values as a new set of env vars?
    OR
  • Merge with existing env vars 1️⃣

Q2: How to we perform any merges and conficts?

  • When the env file contains an existing var, do we:
    • Update/overwrite the existing value? (easiest to implement but not optimal)
    • Skip and keep existing value? (also easy to implement but not optimal) 2️⃣
    • Warn and offer to skip or override? (better solution but a bit more work)
    • Error out (forcing user to delete existing env vars) (least favourite option since a large env would require lots of manual work)
  • When the env file contains an non overridable var do we:
    • Continue / skip, warn user in result dialog 3️⃣
    • Halt, dont apply any of the vars & inform user of error
  • When the env file contains a platform var (e.g. FF_*)
    • Continue / skip, warn user in result dialog 4️⃣
    • Halt, dont apply any of the vars & inform user of error
My chosen direction unless otherwise advised
  • 1️⃣ Merge and keep existing env vars is my preference but I can definitely see the benefit of clearing out what might be LOTS of existing env vars first.
  • 2️⃣ When the env file contains an existing var, skip it and keep existing value. Reasoning: User may apply a template but want to keep pre-set env vars. Warn user in result dialog.
  • 3️⃣ When the env file contains a non overridable var, Continue / skip, warn user in result dialog. Reasoning: As it is not possible to override a locked parent env vars, i cannot think of a reason to halt the import.
  • 4️⃣ When the env file contains a platform var (e.g. FF_*), Continue / skip, warn user in result dialog. Reasoning: As it is not possible to add env vars starting FF_, the i cannot think of a reason to halt the import.

@knolleary
Copy link
Member

  • Importing is adding to the list - not replacing the list.
  • Update any existing values for matching env vars where the template policy permits the value to be changed.
  • Ignore any starting with FF_ as they are not settable by the user.

Steve-Mcl added a commit that referenced this issue Jul 18, 2023
Implements changes outlined in #2372
@Steve-Mcl Steve-Mcl linked a pull request Jul 18, 2023 that will close this issue
11 tasks
@Steve-Mcl
Copy link
Contributor

Verified on staging.

These videos demonstrate the capacities and expectation - may be useful for future reference.

Apologies, no voice over (just some mouse wiggles)

Outline

Each import demonstration shows

  • FF_* env vars are not imported
  • existing locked template entries are not overwritten
  • existing editable template items are imported and overwritten
  • invalid entries are imported but highlighted as bad - for user to correct
  • spaces are supported - when quoted
  • unquoted space padding is trimmed
  • file comments are ignored (additionally, not retained either)

Importing env var file into a devices environment

env-var-import-on-device.mp4

Importing env var file into an instances environment

env-var-import-on-instance.mp4

Importing env var file into a template

env-var-importing-in-template.mp4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:M - 3 Sizing estimation point story A user-oriented description of a feature
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants