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

Add an apache::vhost::proxy define #2169

Merged
merged 4 commits into from
Jun 6, 2022

Conversation

wbclark
Copy link
Contributor

@wbclark wbclark commented Jul 20, 2021

This builds on apache::vhost::fragment with a more specialized type for
a reverse proxy setup. This reuses the same template as apache::vhost
but has data types. The types are stricter, but this leads to a clearer
API. For example, it doesn't accept String and Array[String] but in that
case always wants an Array.

The old parameters on vhost remain and can continue to be used.

@wbclark wbclark requested a review from a team as a code owner July 20, 2021 16:46
@puppet-community-rangefinder
Copy link

apache::vhost::proxy is a type

that may have no external impact to Forge modules.

This module is declared in 175 of 577 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@wbclark
Copy link
Contributor Author

wbclark commented Jul 20, 2021

Discussed with @ekohl and agreed to take over #2167

This PR therefore contains my latest work on it to get the tests into a working state.

@wbclark wbclark force-pushed the proxypass_wclark branch from b5c28d6 to d8fca7a Compare July 20, 2021 17:44
manifests/vhost/proxy.pp Outdated Show resolved Hide resolved
types/vhost/proxypass.pp Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented May 9, 2022

This PR has been marked as stale because it has been open for a while and has had no recent activity. If this PR is still important to you please drop a comment below and we will add this to our backlog to complete. Otherwise, it will be closed in 7 days.

@github-actions github-actions bot added the stale label May 9, 2022
@david22swan
Copy link
Member

@wbclark Apologies for the wait on a review.
Anyway, after taking a look over this pr it for the most part looks good to me, however would be more comfortable if you responded to ekohls comments before I merged it in.

ekohl and others added 2 commits May 27, 2022 07:23
This builds on apache::vhost::fragment with a more specialized type for
a reverse proxy setup. This reuses the same template as apache::vhost
but has data types. The types are stricter, but this leads to a clearer
API. For example, it doesn't accept String and Array[String] but in that
case always wants an Array.

The old parameters on vhost remain and can continue to be used.
Adds thorough documentation for the Apache::Vhost::Proxy
resource type and Apache::Vhost::ProxyPass data type introduced
by the commit b37682. Addresses issues with clarity and
coverage identified in code review.
@wbclark wbclark force-pushed the proxypass_wclark branch from d8fca7a to c3b91c7 Compare May 27, 2022 21:44
@wbclark
Copy link
Contributor Author

wbclark commented May 27, 2022

Thanks very much @ekohl and @david22swan for the feedback, and sorry for the delay in resuming work on this.

I've updated this with major improvements to the documentation for the data type and the defined resource type, in order to address the provided feedback.

@david22swan
Copy link
Member

david22swan commented May 31, 2022

@wbclark Look's like your getting a syntax failure on your code sorry, it's a small one though and an easy fix:

manifests/vhost/proxy.pp - ERROR: there should be a single space between the class or resource name and the next item on line 107 (check: manifest_whitespace_class_name_single_space_after)

One other thing is, could you add a type to the $priority variable, as part of our new commitment to the community standard syntax rules we now require them in all instances.

$priority = undef,

@wbclark
Copy link
Contributor Author

wbclark commented May 31, 2022

@david22swan thanks, I've made the requested updates.

I did these in separate commits for easier reviewing, I assume you'll want to squash before merging. Let me know if you prefer me to do that and force push, or if you'd rather do the squashing on your end during merge. Thanks!

@david22swan
Copy link
Member

david22swan commented May 31, 2022

@wbclark One more quick change sorry, but to the best of my knowledge, $priority should be Variant[Integer,String,Boolean]
Integer and String for passing in input and Boolean for if you wish to pass through false and disable it and indicate no set priority.
Also the value in custom.pp should not be set to optional[], as that is used only when the default value is given as undef.

Thank you though for getting back on this so fast, but I will be off from tomorrow through till Monday so won't be able to get it merged till then sorry.

@wbclark wbclark force-pushed the proxypass_wclark branch from 8a96bdb to 7c58b49 Compare May 31, 2022 16:58
@wbclark
Copy link
Contributor Author

wbclark commented May 31, 2022

Thanks @david22swan -- I'd only seen Integer values for $priority before. I made the update to the requested type. Enjoy the time off and I'll look forward to following up on your return.

@david22swan
Copy link
Member

@wbclark Thanks for putting in the work and getting this so clean. It LGTM now so I'm gonna go ahead and merge.

Also, to clarify about the accepted values, while the apache config only accepts integer values being given, the original examples and default variable both indicate it being passed through as a string, thus necessitating that we accept input in that manner.

Anyway, thanks again for putting in the work and getting it done so fast :)
Looking forward to seeing more

Copy link
Member

@david22swan david22swan 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 merged commit 3619b33 into puppetlabs:main Jun 6, 2022
@wbclark
Copy link
Contributor Author

wbclark commented Jun 6, 2022

Thanks @david22swan and @ekohl ! It's been a pleasure working with you on it.

@david22swan david22swan added feature and removed stale labels Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants