Skip to content

Add rubygems_url property to chef_client_config resource#12724

Merged
johnmccrae merged 1 commit intochef:mainfrom
decoyjoe:feature/add-rubygems_url-to-chef_client_config
Mar 30, 2022
Merged

Add rubygems_url property to chef_client_config resource#12724
johnmccrae merged 1 commit intochef:mainfrom
decoyjoe:feature/add-rubygems_url-to-chef_client_config

Conversation

@decoyjoe
Copy link
Copy Markdown
Contributor

Adds a rubygems_url property to the chef_client_config resource to configure the same such property in client.rb

Related Issue

Fixes #12711

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

  • I have read the CONTRIBUTING document.
  • I have run the pre-merge tests locally and they pass.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commits have been signed-off for the Developer Certificate of Origin.

@decoyjoe decoyjoe requested review from a team as code owners March 25, 2022 20:14
@github-actions github-actions bot added the documentation How do we use this project? label Mar 25, 2022
@decoyjoe decoyjoe force-pushed the feature/add-rubygems_url-to-chef_client_config branch 2 times, most recently from b840842 to da53339 Compare March 25, 2022 20:17
@johnmccrae
Copy link
Copy Markdown
Contributor

@decoyjoe I'm letting this run now but it needs some test cases. Can you add some unit and functional tests to show this doesn't break other things and how it gets handled if someone ignores the setting?

@PrajaktaPurohit
Copy link
Copy Markdown
Contributor

Thank you for your PR! Looks like it needs some chefstyle fixes?

@decoyjoe decoyjoe force-pushed the feature/add-rubygems_url-to-chef_client_config branch from da53339 to 16851b4 Compare March 28, 2022 19:12
@decoyjoe
Copy link
Copy Markdown
Contributor Author

decoyjoe commented Mar 28, 2022

@johnmccrae @PrajaktaPurohit I've added passing spec tests and kitchen tests, and fixed the linting failure.

@johnmccrae
Copy link
Copy Markdown
Contributor

@decoyjoe Can you please replace the single-quotes in your test cases with double-quotes and re-push. (note the linting error above)

@decoyjoe decoyjoe force-pushed the feature/add-rubygems_url-to-chef_client_config branch 2 times, most recently from 75ed7fc to 9371261 Compare March 28, 2022 21:21
@decoyjoe
Copy link
Copy Markdown
Contributor Author

Oops, force of habit. Fixed @johnmccrae

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this probably isn't necessary. the default should probably be nil, either way it gets handled correctly by the code in the template, but nil is a better "unset" value.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@lamont-granquist Is your recommendation to set default: nil or to remove the default: entirely?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can omit default and it'll gain a default of nil. That'd be preferable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@chef chef deleted a comment from johnmccrae Mar 30, 2022
Signed-off-by: Joseph Larionov <jlarionov@webmd.net>
@decoyjoe decoyjoe force-pushed the feature/add-rubygems_url-to-chef_client_config branch from 9371261 to a8a6887 Compare March 30, 2022 21:16
Copy link
Copy Markdown
Contributor

@johnmccrae johnmccrae left a comment

Choose a reason for hiding this comment

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

looks good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation How do we use this project?

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add rubygems_url property to chef_client_config resource

4 participants