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 ability for the tentacle resource to setup firewall rule for listening tentacles #43

Merged
merged 1 commit into from Aug 8, 2016

Conversation

spuder
Copy link
Contributor

@spuder spuder commented Aug 8, 2016

Adds firewall for tentacle
Since polling tentacles don't need the firewall opened, it only applies to listening tentacles.
Defaults to 'false'.

TODO

  • Should this default to true or false?
  • Should this be moved to a resource parameter?

False is safer for existing cookbook users, but is bound to trip up some new users. I've added a new section to the readme making this more obvious.

Because this is an attribute, some users might have default['octopus'] while others would have default['my-octopus'] in their wrapper cookbook

default['my-octopus']['tentacle']['instance']['polling']
default['octopus']['tentacle']['instance']['manage_firewall'] = true

@spuder spuder changed the title Adds firewall settings [wip] Adds firewall settings Aug 8, 2016
If you manage tentacle firewall though group policy leave 'manage_firewall' `false`
If you want the cookbook to manage the firewall, set `true`

node['octopus']['tentacle']['instance']['manage_firewall'] = false
Copy link
Member

Choose a reason for hiding this comment

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

I actually have pr #42 to remove attribute files from the cookbook just because they are not used. Everything is using a resource. When this cookbook was first created we did use the attributes but now we do not

@brentm5
Copy link
Member

brentm5 commented Aug 8, 2016

It looks good I think I would move it to a resource parameter and not include it in the attributes. In addition I think defaulting it to false is the correct way since it is the legacy behavior to not manage it. We can maybe default it in the future once this goes to 1.0.0

@brentm5 brentm5 added this to the v1.0.0 milestone Aug 8, 2016
@spuder spuder force-pushed the feature/firewall branch 2 times, most recently from 8862601 to bd6683f Compare August 8, 2016 19:04
@spuder spuder changed the title [wip] Adds firewall settings Adds firewall settings Aug 8, 2016
@spuder
Copy link
Contributor Author

spuder commented Aug 8, 2016

I've gotten rid of the attributes and made a configure_firewall resource parameter.
I've also squashed and removed 'wip' from the title.

@spuder spuder changed the title Adds firewall settings Adds listening tentacle firewall settings Aug 8, 2016
@brentm5 brentm5 changed the title Adds listening tentacle firewall settings Add ability for the tentacle resource to setup firewall rule for listening tentacles Aug 8, 2016
@spuder spuder force-pushed the feature/firewall branch 2 times, most recently from 56f5849 to 001dfd9 Compare August 8, 2016 21:19
- :polling: Whether this Octopus Deploy Instance is a polling tentacle (Defaults to False)
- :cert_file: Where to export the Octopus Deploy Instance cert (Defaults to C:\Octopus\tentacle_cert.txt)
- :upgrades_enabled: Whether to upgrade or downgrade the tentacle version if the windows installer version does not match what is provided in the resource. (Defaults to True)
- :server: Url to Octopus Deploy Server (e.g https://octopus.example.com)
- :api_key: Api Key used to register Tentacle to Octopus Server
- :roles: Array of roles to apply to Tentacle when registering with Octopus Deploy Server (e.g ["web-server","app-server"])
- :environment: Which environment the Tentacle will become part of when registering with Octopus Deploy Server
- :environment: Which environment the Tentacle will become part of when registering with Octopus Deploy Server (Defaults to node.chef_environment )
Copy link
Member

Choose a reason for hiding this comment

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

Was this meant to be in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, that was supposed to be part of #45.

I can separate them if you want.

Copy link
Member

Choose a reason for hiding this comment

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

If you don't mind. Trying to be somewhat good with feature merges

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, hold off merging until I resquash

Squashed commits:
[bd6683f] Adds firewall settings

Adds firewall clarification to readme

Moves firewall from attribute to resource parameter

Remove attributes from repo as all interaction is done via resources (cvent#42)

- This repository provides resources so attributes are not needed
@brentm5 brentm5 merged commit 860051e into cvent:master Aug 8, 2016
@spuder spuder deleted the feature/firewall branch August 8, 2016 21:43
@brentm5 brentm5 mentioned this pull request Aug 8, 2016
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.

None yet

2 participants