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

[THREESCALE-6116] Conditional policy: disable UI field #1530

Closed
wants to merge 1 commit into from

Conversation

jlledom
Copy link

@jlledom jlledom commented Jan 27, 2025

THREESCALE-6116 is assigned to porta but I think it belongs to here.

I don't think we'll be able to edit the conditional policy from UI because this is a corner case where we need to recursively add a jsonschema form inside another. I'm not sure that's possible but in any case it would take much more resources than the issue deserves IMO.

After a discussion in the comments, it was agreed to just add a notice about this policy being managed by operator. In my opinion, the right place to do this is the policy schema in Apicast.

After taking a look, I think we don't actually need to disable the whole policy from UI, only the recursive field policy_chain. I tested the condition field and it works fine and can be edited from UI without problems.

Since I already made some changes in the policy locally, I thought it would be good to share them.

This is how the form looks now:

image

@jlledom jlledom self-assigned this Jan 27, 2025
@jlledom jlledom requested a review from a team as a code owner January 27, 2025 15:43
@jlledom jlledom requested a review from tkan145 January 27, 2025 15:44
@tkan145
Copy link
Contributor

tkan145 commented Jan 28, 2025

Hi @jlledom, thanks for working on this.

policies_list/x.x.x/policies.json is something that we will generate right before each release. The actual configure for the policy is here

After taking a look, I think we don't actually need to disable the whole policy from UI, only the recursive field policy_chain. I tested the condition field and it works fine and can be edited from UI without problems.

Yes you can modify other fields of the policy, but it is unusable without the nested policy chain, so I think we have a few options:

  1. Remove this policy from the admin portal completely (remove apicast-policy.json file from APIcast) and update the documentation to mention that this policy is only configurable via the operator's CR
  2. Remove all fields from the jsonschema and add a notice (update apicast-policy.json file) that this policy is only configurable via operator's CR

Once we agree on how we want to move forward. I will create a new policies_list/x.x.x/policies.json for the 2.16 release and that will reflect in the admin portal without any code changes on your part. WDYT?

@jlledom
Copy link
Author

jlledom commented Jan 28, 2025

policies_list/x.x.x/policies.json is something that we will generate right before each release. The actual configure for the policy is here

Good to know.

Yes you can modify other fields of the policy, but it is unusable without the nested policy chain, so I think we have a few options:

Yes, the user would need to update policy_chain from the API, but still could modify other fields from UI. Though I understand it can be confusing for the user.

1. Remove this policy from the admin portal completely (remove apicast-policy.json file from APIcast) and update the documentation to mention that this policy is only configurable via the operator's CR

I'm not sure who has to decide, but I vote for this.

Copy link
Contributor

@dfennessy dfennessy left a comment

Choose a reason for hiding this comment

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

/lgtm

@tkan145
Copy link
Contributor

tkan145 commented Jan 30, 2025

thanks @dfennessy

@jlledom let go with option 1. Do you might to update this PR? revert this current change and make a new commit deleting the file here. Or would you prefer to close this PR and let me handle it?

@jlledom
Copy link
Author

jlledom commented Jan 30, 2025

@tkan145 I'll close this and let you handle it. Thanks!

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.

3 participants