-
Notifications
You must be signed in to change notification settings - Fork 235
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
Default requireApproverJustification to false before diffing #3307
base: master
Are you sure you want to change the base?
Default requireApproverJustification to false before diffing #3307
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test to reflect this change?
acab2e7
to
18ce2b6
Compare
Done! |
I saw you changed the test infra for PAM. We cannot do it because 1. if it is an infra change, we should PR review the infra separately and add test coverage to validate the infra change; 2. Adding I suggest we add a scenario test, and design the steps that can reflect the expected outcome of this fix. |
18ce2b6
to
e271ec1
Compare
e271ec1
to
17f4a44
Compare
Change description
Fixes the issue that there is always a diff when
spec.ApprovalWorkflow.ManualApprovals.RequireApproverJustification
is set tofalse
in the KCC manifest. This issue happens becausefalse
is the default value for this field, and the default value is not returned by the PAM API.Supported no change step for PrivilegedAccessManagerEntitlement to verify this change in mock test. (Credit to #3045 by @acpana.)
Also updated the testdata version for PAM from v1alpha1 to v1beta1.
Tests you have done
make ready-pr
to ensure this PR is ready for review.