Conversation
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
|
Jenkins test this please |
fatmcgav
left a comment
fatmcgav
left a comment
There was a problem hiding this comment.
@jtbrush56 Thank you for taking the time to make these additions.
A few comments that need addressing.
I'd also like to see the new properites added to the unit tests.[1][2]
Also, could you add an example to the README.md[3]
[1] https://github.com/elastic/puppet-elasticsearch/blob/master/spec/unit/type/elasticsearch_snapshot_repository_spec.rb
[2] https://github.com/elastic/puppet-elasticsearch/blob/master/spec/unit/provider/elasticsearch_snapshot_repository/ruby_spec.rb
[3] https://github.com/elastic/puppet-elasticsearch#snapshot-repositories
| :base_path => api_object['settings']['base_path'], | ||
| :client => api_object['settings']['client'], | ||
| :readonly => api_object['settigns']['readonly'], | ||
| :location_mode => api_object['settings']['location_mode'], |
There was a problem hiding this comment.
These new fields will need to be added as optional fields on the generate_body method below.
| newproperty(:location) do | ||
| desc 'Repository location' | ||
| end | ||
|
|
There was a problem hiding this comment.
Looks like rubocop is complaining about trailing whitespace here.
| end | ||
|
|
||
| newproperty(:container) do | ||
| defaultto 'elasticsearch-snapshots' |
There was a problem hiding this comment.
I don't think we should set a default value for this field. Instead it should be a conscious decision by the user as to what they want to call their Azure container.
| # | ||
| # @param location | ||
| # Location of snapshots. Mandatory | ||
| # Location of snapshots. |
There was a problem hiding this comment.
This field is still mandatory when type => "fs", so we should state that.
manifests/snapshot_repository.pp
Outdated
| String $location, | ||
| Optional[String] $location = undef, | ||
| Enum['absent', 'present'] $ensure = 'present', | ||
| Optional[String] $client = undef, |
There was a problem hiding this comment.
Can you sort out the alignment on these lines? Should all align on =.
|
updated with requested changes. |
fatmcgav
left a comment
fatmcgav
left a comment
There was a problem hiding this comment.
It looks like rubocop is still complaining about trailing whitespace on https://github.com/elastic/puppet-elasticsearch/pull/1087/files#diff-9240f318e7e1ac50308c77dfa32622ddR49-R54
You should be able to test rubocop locally by running bundle exec rake rubocop.
|
Dear @jtbrush56, thanks for the PR! This is Vox Pupuli Tasks, your friendly Vox Pupuli Github Bot. I noticed that your pull request has CI failures. Can you please have a look at the failing CI jobs? |
|
Dear @jtbrush56, thanks for the PR! This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
3 participants
Pull request acceptance prerequisites: