Skip to content

(CAT-1417) Nested require support for authz_core mod #2460

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
Merged
merged 2 commits into from
Sep 21, 2023
Merged

(CAT-1417) Nested require support for authz_core mod #2460

merged 2 commits into from
Sep 21, 2023

Conversation

Ramesh7
Copy link
Contributor

@Ramesh7 Ramesh7 commented Sep 13, 2023 •
edited
Loading

Summary

  • Adding support for Authz_core module nested require statement.
  • Regenerating REFERENCE.md

Additional Context

#2315

Related Issues (if any)

#2315

Checklist

  • 🟢 Spec tests.
  • 🟢 Acceptance tests.
  • Manually verified. (For example puppet apply)

@Ramesh7 Ramesh7 added the wip Work In Progress label Sep 13, 2023
@Ramesh7 Ramesh7 force-pushed the CAT-1417-nested-require-for-authz-mod branch 4 times, most recently from 39bf331 to 4c2f279 Compare September 13, 2023 11:54
bastelfreak
bastelfreak reviewed Sep 15, 2023
View reviewed changes
bastelfreak
bastelfreak reviewed Sep 15, 2023
View reviewed changes
@Ramesh7 Ramesh7 force-pushed the CAT-1417-nested-require-for-authz-mod branch 2 times, most recently from 1b4316a to 504c409 Compare September 18, 2023 06:08
@Ramesh7
Copy link
Contributor Author

Ramesh7 commented Sep 18, 2023

Addressed with all changes and now ready for review.

@Ramesh7 Ramesh7 removed the wip Work In Progress label Sep 18, 2023
@Ramesh7 Ramesh7 marked this pull request as ready for review September 18, 2023 06:11
@Ramesh7 Ramesh7 requested review from ekohl, smortex and a team as code owners September 18, 2023 06:11
@Ramesh7 Ramesh7 force-pushed the CAT-1417-nested-require-for-authz-mod branch from 504c409 to d0f9034 Compare September 18, 2023 06:21
smortex
smortex reviewed Sep 18, 2023
View reviewed changes
Copy link
Collaborator

@smortex smortex left a comment

Choose a reason for hiding this comment

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

That looks great, I added in-line notes for a few rather minor points.

I am also wondering if we can add an argument to the recursive function so that the generated code has indentation?

@Ramesh7 Ramesh7 force-pushed the CAT-1417-nested-require-for-authz-mod branch 6 times, most recently from ded5fc9 to 37d7863 Compare September 18, 2023 12:13
@Ramesh7
Copy link
Contributor Author

Ramesh7 commented Sep 18, 2023

That looks great, I added in-line notes for a few rather minor points.

I am also wondering if we can add an argument to the recursive function so that the generated code has indentation?

Have fixed the indentation.

gavindidrichsen
gavindidrichsen previously approved these changes Sep 18, 2023
View reviewed changes
Copy link

@gavindidrichsen gavindidrichsen left a comment

Choose a reason for hiding this comment

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

LGTM!

david22swan
david22swan previously approved these changes Sep 18, 2023
View reviewed changes
smortex
smortex reviewed Sep 18, 2023
View reviewed changes
Copy link
Collaborator

@smortex smortex left a comment

Choose a reason for hiding this comment

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

A few style comments and LGTM!

@Ramesh7 Ramesh7 dismissed stale reviews from david22swan and gavindidrichsen via 0e9dd68 September 18, 2023 17:21
@Ramesh7 Ramesh7 force-pushed the CAT-1417-nested-require-for-authz-mod branch from 37d7863 to 0e9dd68 Compare September 18, 2023 17:21
smortex
smortex previously approved these changes Sep 18, 2023
View reviewed changes
Copy link
Collaborator

@smortex smortex left a comment

Choose a reason for hiding this comment

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

LGTM, just the documentation missing makes me skeptical.

@Ramesh7 Ramesh7 force-pushed the CAT-1417-nested-require-for-authz-mod branch from 0e9dd68 to c179f67 Compare September 19, 2023 15:38
@Ramesh7 Ramesh7 mentioned this pull request Sep 19, 2023
Open
@Ramesh7 Ramesh7 force-pushed the CAT-1417-nested-require-for-authz-mod branch 2 times, most recently from 2686955 to b9c9405 Compare September 20, 2023 04:42
@malikparvez malikparvez self-requested a review September 20, 2023 05:44
malikparvez
malikparvez previously approved these changes Sep 20, 2023
View reviewed changes
Copy link
Member

@malikparvez malikparvez left a comment

Choose a reason for hiding this comment

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

lgtm

@Ramesh7 Ramesh7 force-pushed the CAT-1417-nested-require-for-authz-mod branch from b9c9405 to 0a64748 Compare September 20, 2023 12:38
@Ramesh7 @smortex
Update manifests/vhost.pp
c9cc80d 
Co-authored-by: Romain Tartière <romain@blogreen.org>
@Ramesh7 Ramesh7 force-pushed the CAT-1417-nested-require-for-authz-mod branch from 0a64748 to c9cc80d Compare September 21, 2023 00:23
smortex
smortex approved these changes Sep 21, 2023
View reviewed changes
Copy link
Collaborator

@smortex smortex left a comment

Choose a reason for hiding this comment

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

LGTM!

REFERENCE.md
Comment on lines -7579 to -7580
lint:ignore:parameter_documentation
lint:endignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removing 💩 from the doc. Awesome 💯

@Ramesh7 Ramesh7 merged commit ac1b7b4 into main Sep 21, 2023
@Ramesh7 Ramesh7 deleted the CAT-1417-nested-require-for-authz-mod branch September 21, 2023 04:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bastelfreak bastelfreak bastelfreak left review comments

@smortex smortex smortex approved these changes

@gavindidrichsen gavindidrichsen gavindidrichsen left review comments

@david22swan david22swan david22swan left review comments

@malikparvez malikparvez malikparvez left review comments

@ekohl ekohl Awaiting requested review from ekohl ekohl is a code owner

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

Successfully merging this pull request may close these issues.
6 participants
@Ramesh7 @smortex @bastelfreak @gavindidrichsen @david22swan @malikparvez