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

Enable marking config-app fields as read only. #310

Merged
merged 1 commit into from
Apr 7, 2020
Merged

Enable marking config-app fields as read only. #310

merged 1 commit into from
Apr 7, 2020

Conversation

kurtismullins
Copy link
Contributor

@kurtismullins kurtismullins commented Apr 3, 2020 •
edited

Description of Changes

This change-set allows Users to mark certain fields in the Configuration application as read-only. This is helpful for cases where certain fields may be managed by an external resource, such as the Quay Operator.

The data should still be available when read from an existing configuration, but the User should not be allowed to manually edit it through the UI.

The following options are currently supported:

  • Redis
  • Storage Not used at the moment. Can be implemented in the future.
  • Hostname

Changes:

  • When running the Quay Configuration app, the following environment variable and options can be used to specify certain components as read-only: READ_ONLY_FIELDS=redis,hostname

Issue:

TESTING

  • Run the Quay Configuration application with all combinations of the mentioned environment variables. Ensure that in each case, the expected component cannot be modified by the User. Ensure that when reading the configuration from an existing cluster, the data is there (but not modifiable)

BREAKING CHANGE

n/a

Reviewer Checklist

  • It works!
  • Comments provide sufficient explanations for the next contributor
  • Tests cover changes and corner cases
  • Follows Quay syntax patterns and format

@kurtismullins
Copy link
Contributor Author

@josephschorr or @alecmerdler

Would you be able to lend me a hand? I am not quite sure how to bridge the gap between these values existing in window.__config and turning them into feature flags, or otherwise making them accessible to the AngularJS templating.

The end result that I'm shooting for is to be able to show, but not allow modifying, the configuration for Redis, the Hostname, or the Storage, if the environment variable is set.

Feel free to let me know if I'm going about this the wrong way or if it's non-trivial. It's related to the "Day 2" Operator work.

Thanks!

@kurtismullins kurtismullins changed the title [PROJQUAY-319] Enable marking config-app fields as read only. Enable marking config-app fields as read only. Apr 3, 2020
@kurtismullins kurtismullins marked this pull request as ready for review April 3, 2020 19:42
josephschorr
josephschorr reviewed Apr 3, 2020
View reviewed changes
config_app/config_endpoints/setup_web.py Outdated Show resolved Hide resolved
config_app/config_endpoints/setup_web.py Outdated Show resolved Hide resolved
config_app/js/core-config-setup/config-setup-tool.html Outdated Show resolved Hide resolved
alecmerdler
alecmerdler suggested changes Apr 3, 2020
View reviewed changes
Copy link
Contributor

@alecmerdler alecmerdler left a comment

Choose a reason for hiding this comment

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

Deployed it and fields were still modifiable. Saw this error in the browser console: https://code.angularjs.org/1.5.3/docs/error/$compile/nonassign?p0=undefined&p1=isReadonly&p2=configStringField

@kurtismullins
Copy link
Contributor Author

Thanks for the tips on the one-time binding, @alecmerdler!

When I run the configuration locally, passing in my environment variables, using the command:

CONFIG_READ_ONLY_FIELDS=redis,hostname python -m config_app.config_application

I successfully see that my warnings are displayed and that the configuration items are read-only. I still need to test this on a cluster where I am modifying an existing configuration.

read-only-redis
read-only-hostname

When I do not set a feature to be read-only, I still see the big warning. I think the ng-if statements are not working as expected. Any suggestions @josephschorr?

hostname-warning-but-not-set

josephschorr
josephschorr reviewed Apr 6, 2020
View reviewed changes
config_app/config_endpoints/setup_web.py Outdated Show resolved Hide resolved
config_app/config_endpoints/setup_web.py Outdated Show resolved Hide resolved
config_app/js/config-field-templates/config-string-field.html Outdated Show resolved Hide resolved
config_app/js/core-config-setup/core-config-setup.js Outdated Show resolved Hide resolved
kurtismullins
kurtismullins commented Apr 7, 2020
View reviewed changes
config.py Outdated Show resolved Hide resolved
@kurtismullins
Copy link
Contributor Author

This is ready for another review-round. As far as I can tell, it is working successfully.

Here are screenshots demonstrating it works successfully when modifying a cluster's existing configuration.

redis-read-only
hostname-read-only

josephschorr
josephschorr approved these changes Apr 7, 2020
View reviewed changes
Copy link
Contributor

@josephschorr josephschorr left a comment

Choose a reason for hiding this comment

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

LGTM

alecmerdler
alecmerdler reviewed Apr 7, 2020
View reviewed changes
config_app/c_app.py
@@ -11,6 +11,7 @@
from config_app.config_util.config import get_config_provider
from util.security.instancekeys import InstanceKeys


Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

alecmerdler
alecmerdler reviewed Apr 7, 2020
View reviewed changes
config_app/js/config-field-templates/config-numeric-field.html
@@ -1,6 +1,7 @@
<div class="config-numeric-field-element">
<form name="fieldform" novalidate>
<input type="number" class="form-control" placeholder="{{ placeholder || '' }}"
ng-model="bindinginternal" ng-trim="false" ng-minlength="1" required>
ng-model="bindinginternal" ng-trim="false" ng-minlength="1" required
ng-readonly="::isReadonly">
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@kurtismullins kurtismullins merged commit 1a55e35 into quay:master Apr 7, 2020
@kurtismullins kurtismullins deleted the PROJQUAY-319 branch April 7, 2020 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josephschorr josephschorr josephschorr approved these changes

@alecmerdler alecmerdler alecmerdler approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
3 participants
@kurtismullins @josephschorr @alecmerdler