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 env var values to be hidden in UI #5043

Merged
merged 27 commits into from
Jan 22, 2025

Conversation

cstns
Copy link
Contributor

@cstns cstns commented Jan 21, 2025

Description

Allows setting of hidden env vars, once set they can only be updated or removed

  • applies to both remote and hosted instances
  • duplicating hosted instances preserves varhidden state
  • other limitations described in closes Set Env-vars to be hidden after creation #4835
  • updated error logging and validation to include env vars names starting with numbers
  • fixes validation recursion
  • fixes removal of unsaved vars

Related Issue(s)

closes #4835

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

  • Includes a DB migration? -> add the area:migration label

@cstns cstns self-assigned this Jan 21, 2025
Copy link

codecov bot commented Jan 21, 2025

Codecov Report

Attention: Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 78.41%. Comparing base (0d5c9e1) to head (9d8b069).
Report is 37 commits behind head on main.

Files with missing lines Patch % Lines
forge/routes/api/project.js 66.66% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5043   +/-   ##
=======================================
  Coverage   78.41%   78.41%           
=======================================
  Files         329      329           
  Lines       15480    15494   +14     
  Branches     3574     3579    +5     
=======================================
+ Hits        12138    12150   +12     
- Misses       3342     3344    +2     
Flag Coverage Δ
backend 78.41% <87.50%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@knolleary
Copy link
Member

@cstns can you add a screenshot of this in action? Helpful to see as part of the initial review, but can also be reused in changelog.

@cstns
Copy link
Contributor Author

cstns commented Jan 21, 2025

image

One thing that I should have probably done but can be added later on is to prompt an alert or make the user aware that once an env var is marked as hidden he can't make it visible again, only update it's value through the UI or possibly delete it.

I'll add a demo as well when done

@cstns cstns requested a review from knolleary January 21, 2025 17:04
@knolleary knolleary changed the title 4835 set env vars to be hidden after creation Allow env var values to be hidden in UI Jan 22, 2025
@knolleary
Copy link
Member

Doing some testing with a Device - any env var with a hidden value ends up blank on the device. I haven't dug through the whole PR yet, but this change may be to blame as its code that's reused when sending info on the env vars to the front end as well as down to the device.

@cstns
Copy link
Contributor Author

cstns commented Jan 22, 2025

but this change may be to blame as its code that's reused when sending info on the env vars to the front end as well as down to the device.

You are right, i'll move this to be filtered on the controller side. The same thing is happening to instance vars

@knolleary
Copy link
Member

Verified its working properly now.

I'm wondering if the eye icon is clear enough as to what it does. Could we add a tooltip explaining its purpose?

@cstns
Copy link
Contributor Author

cstns commented Jan 22, 2025

can you take a quick look over 9d8b069?

Copy link
Member

@knolleary knolleary 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.

@cstns cstns merged commit ac7ce53 into main Jan 22, 2025
20 checks passed
@cstns cstns deleted the 4835_set-env-vars-to-be-hidden-after-creation branch January 22, 2025 15:24
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.

Set Env-vars to be hidden after creation
2 participants