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.

Sign up for GitHub

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

Jump to bottom

Mark variables set by environment variable as read only in the web interface #2787

Merged
merged 5 commits into from
Oct 29, 2023
Merged

Mark variables set by environment variable as read only in the web interface #2787

merged 5 commits into from
Oct 29, 2023

Conversation

PromoFaux
Copy link
Member

@PromoFaux PromoFaux commented Oct 27, 2023 •
edited
Loading

What does this PR aim to accomplish?:

If a config item is already set by environment variable (this making it read-only) - then we disable the input on the web interface to prevent confusion.

image

By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)
  • I have read the above and my PR is ready for review. Check this box to confirm

@PromoFaux PromoFaux requested a review from a team October 27, 2023 14:50
@PromoFaux PromoFaux force-pushed the new/env_vars_readonly branch 3 times, most recently from 80cf75a to faf9c2a Compare October 27, 2023 15:13
@PromoFaux PromoFaux changed the title New/env vars readonly Mark variables set by environment variable as read only in the web interface Oct 28, 2023
@PromoFaux
Copy link
Member Author

With the latest commit(s)

image

DL6ER
DL6ER reviewed Oct 29, 2023
View reviewed changes
Copy link
Member

@DL6ER DL6ER left a comment

Choose a reason for hiding this comment

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

Screenshot from 2023-10-29 06-40-04

No "restart required" icon here

@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

DL6ER and others added 4 commits October 29, 2023 15:21
@DL6ER @PromoFaux
Indicate config settings forced by env vars as read-only variables by…
619ea8f 
… marking them with a symbol and disabling the elements

Signed-off-by: DL6ER <dl6er@dl6er.de>
@PromoFaux
Disable settings inputs if they are already set by environment variab…
fef945c 
…le. Add a padlock icon to the setting section title if any of the settings contained therin are set by environment variable
@PromoFaux
In addition to adding the readonly icon, we can also add the other ic…
66401ee 
…ons with teh same method.

Signed-off-by: Adam Warner <me@adamwarner.co.uk>
@PromoFaux
The CI does not like nested IF statements with a single action inside
b2572c0 
Signed-off-by: Adam Warner <me@adamwarner.co.uk>
@github-actions
Copy link
Contributor

Conflicts have been resolved.

@PromoFaux
Apply the same treatment to the local DNS records page.
188f8ee 
Refactor of the javascript to reduce code deduplication
Fix datatable refresh after adding/removing an entry
@PromoFaux PromoFaux requested a review from DL6ER October 29, 2023 15:31
DL6ER
DL6ER approved these changes Oct 29, 2023
View reviewed changes
Copy link
Member

@DL6ER DL6ER left a comment

Choose a reason for hiding this comment

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

It works as expected. I just checked on the string literals (...) and see that they have been introduced in ECMAScript 6, so Firefox 54 and Chrome 58 👍

@PromoFaux PromoFaux merged commit efc52cd into development-v6 Oct 29, 2023
7 checks passed
@PromoFaux PromoFaux deleted the new/env_vars_readonly branch October 29, 2023 17:42
@rdwebdesign
Copy link
Member

NOTE:

If we set the webpassword via environment variable in a container, the command pihole setpassword won't be able to change the password (as expected), but no error message is shown on the command line.
Actually, it shows a success message: [✓] New password set, but the password wasn't changed.

This should be fixed to avoid confusion.

@DL6ER
Copy link
Member

DL6ER commented Nov 4, 2023

On changing, pihole-FTL --config webserver.api.password ABCD should return

Config option webserver.api.password is read-only (set via environmental variable)

with exit code 5

@DL6ER DL6ER mentioned this pull request Nov 4, 2023
Closed
5 tasks
@yubiuser
Copy link
Member

yubiuser commented Nov 4, 2023

I think this is fixed by pi-hole/pi-hole#5465

@yubiuser
Copy link
Member

yubiuser commented Nov 4, 2023

Commit 66401ee from this PR should have added the advanced icon to all advanced level settings. However, at least for some this does not work. E.g. DNS/Conditional Forwarding oder DNS/Rate limiting. Maybe this need to be fixed in FTL

@yubiuser yubiuser mentioned this pull request Nov 5, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DL6ER DL6ER DL6ER approved these changes

Assignees
No one assigned
Labels
Pi-hole v6.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@PromoFaux @rdwebdesign @DL6ER @yubiuser