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

Initial implementaton of ClientEnforcementPolicyBasicAuth #97

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Initial implementaton of ClientEnforcementPolicyBasicAuth #97

wants to merge 3 commits into from

Conversation

djizatt-avio
Copy link
Contributor

@djizatt-avio djizatt-avio commented Oct 30, 2024 •
edited
Loading

  • The ClientEnforcementPolicyCustom was existant, but didn't seem to be utilized anywhere
  • I renamed it to ClientEnforcementPolicyCustomAuth to align with the ClientEnforcementPolicyBasicAuth naming standards
  • Added minimum and full policy testing within PolicyContextTest.grooy
  • Updated allOptions.groovy with the new policy configuration
  • I have only tested this within Unit Testing

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 30, 2024 •
edited
Loading

Test Results

325 tests  +3   325 ✅ +3   1m 58s ⏱️ -3s
 40 suites ±0     0 💤 ±0 
 40 files   ±0     0 ❌ ±0 

Results for commit 3447379. ± Comparison against base commit 3931384.

♻️ This comment has been updated with latest results.

@manikmagar manikmagar requested a review from a team as a code owner November 19, 2024 03:38
manikmagar
manikmagar reviewed Nov 19, 2024
View reviewed changes
w Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this diff file intentionally added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was my first time working on the deployer, I think I included this file by mistake, as to my knowledge it isn't needed. I can remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also need to test this change with a deployment to the AVIO IT Dev space before merging.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. @djizatt-avio please remove this file. If the PR is not ready, you can convert it to Draft for now.

@manikmagar
Copy link
Contributor

@djizatt-avio does this PR supersede #26?

@djizatt-avio
Copy link
Contributor Author

@manikmagar I believe it does. I saw that PR as well, but I saw it was from 2021 and the updates in my PR seem to conform better to the current standards utilized within the deployer.

@djizatt-avio djizatt-avio marked this pull request as draft November 21, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manikmagar manikmagar manikmagar left review comments

@adesjardin adesjardin Awaiting requested review from adesjardin

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@djizatt-avio @manikmagar