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

updated documentation to conform with REFERENCE.md standard for forge #311

Merged
merged 2 commits into from
Oct 27, 2018
Merged

updated documentation to conform with REFERENCE.md standard for forge #311

merged 2 commits into from
Oct 27, 2018

Conversation

danquack
Copy link
Contributor

Pull Request (PR) description

In order to match other voxpupuli forge modules, i've updated all the commented code to utilize the REFERENCE.md structure.

This Pull Request (PR) fixes the following issues

Dan33l
Dan33l requested changes Oct 26, 2018
View reviewed changes
Copy link
Member

@Dan33l Dan33l left a comment

Choose a reason for hiding this comment

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

Hi @danquack . Thank you for the PR.

I added some in line comments.

Declaring a class or a defined type as private, is more than producing markdown file. So , if you are interested by the topic, it looks better to work on it with an other PR.

Several tests failed. They have to pass before PR is merged.

manifests/config.pp Outdated
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
# @api private
Copy link
Member

Choose a reason for hiding this comment

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

This is more than documentation.

If you are interested by the topic, i propose you work on it in an other PR.

manifests/client.pp Outdated
@@ -203,7 +53,7 @@
Boolean $persist_key = true,
Boolean $persist_tun = true,
String $port = '1194',
String $proto = 'tcp',
Enum['tcp', 'udp'] $proto = 'tcp',
Copy link
Member

Choose a reason for hiding this comment

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

Nice. \o/

manifests/install.pp Outdated
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
# @api private
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here, it is more than documentation.

manifests/client.pp Outdated
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#
define openvpn::client (
String $server,
String $compression = 'comp-lzo',
Copy link
Member

Choose a reason for hiding this comment

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

What are possible values ?

manifests/service.pp Outdated
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
# @api private
Copy link
Member

Choose a reason for hiding this comment

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

Same here, more than documentation.

Dan33l
Dan33l requested changes Oct 26, 2018
View reviewed changes
@danquack
Copy link
Contributor Author

danquack commented Oct 26, 2018 •
edited
Loading

@Dan33l Any idea as to why the tests would be failing? I reverted every string to enum change I had made except the TCP/UDP one that you commented on, but the tests still fail. I don't see anything in the tests that would cause this, but seems like everything build broken? Perhaps I should back out the tcp/udp as well? Reverting all changes to non currently commented code breaks it as well....

@danquack danquack changed the title added documentation - REFERENCE.md for forge. Updated types to match doc updated documentation to conform with REFERENCE.md standard for forge Oct 26, 2018
@Dan33l
Copy link
Member

Dan33l commented Oct 26, 2018 •
edited
Loading

@danquack Many tests are failing because of default value. It must be also a string :
Enum['1','2','3'] $status_version = '1', .

  • with = '1', default value is a String.
  • with = 1, default value is an Integer and can not match with Enum.

@danquack
Copy link
Contributor Author

danquack commented Oct 26, 2018 •
edited
Loading

@Dan33l good catch. Now that seemed to fix it, let me rebase, squash and add back the code I took away.

@danquack danquack force-pushed the documentation branch 2 times, most recently from f64f202 to aaad847 Compare October 26, 2018 22:41
@danquack
Copy link
Contributor Author

danquack commented Oct 26, 2018 •
edited
Loading

@Dan33l Let me know if theres anything else you want me to change!

Dan33l
Dan33l approved these changes Oct 27, 2018
View reviewed changes
Copy link
Member

@Dan33l Dan33l left a comment

Choose a reason for hiding this comment

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

Thanks !

@Dan33l Dan33l closed this Oct 27, 2018
@Dan33l Dan33l reopened this Oct 27, 2018
@Dan33l Dan33l merged commit fb3381e into voxpupuli:master Oct 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dan33l Dan33l Dan33l approved these changes

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
@danquack @Dan33l