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

Upgrade to Chef 13.9+ #57

Merged
merged 22 commits into from Aug 8, 2018

Conversation

Projects
None yet
2 participants
@martinisoft
Member

martinisoft commented Jun 12, 2018

This is a partial re-write that started from a foodcritic warning FC120: Do not set the name property directly on a resource: which snowballed into an upgrade to add Chef 13+ support and overhaul a bunch of issues I found when digging around.

  • Resolved #56 by renaming the 'name' property
  • Re-documented both resource properties in the README and updated/fixed examples
  • Renamed certificate_common_name property to common_name and documented this
  • Added in a way to allow the user to provide their own private key in the case where someone provides their own custom CSR for a certificate
  • Marked access_token and private_key_pem as sensitive properties
  • Removed the kind_of key in the properties since it is now removed
  • Changed expires_on in certificate resource to a string so it can be a parsable date for ease of use
  • Added CAA record type for dnsimple_record resource
  • Removed certificate test suite due to the fact it's not easily testable via Sandbox. Unit testing will suffice for now until we can figure out a suitable alternative

martinisoft added some commits Jun 12, 2018

@martinisoft martinisoft requested a review from onlyhavecans Jun 12, 2018

property :ttl, kind_of: Integer, default: 3600
property :priority, kind_of: Integer
property :regions, kind_of: [NilClass, Array], default: nil
property :record_name, String, default: ''

This comment has been minimized.

@onlyhavecans

onlyhavecans Jun 12, 2018

Contributor

Why make record_name no longer be a name_property?

This comment has been minimized.

@martinisoft

martinisoft Jun 12, 2018

Member

Record name can be blank which refers to the apex so this can be handled two ways:

  1. Keep record_name as a name property, but leave explicit instructions that this must be set to blank to be on the apex, otherwise it will use the resource name as the record name by default.
  2. Keep it without being a name property and it defaults to the apex unless it is changed and thus being more explicit in the properties.
Remove install certificate suite
Per documentation and developer discussion, we don't issue valid SSL
certificates in the Sandbox environment so this cannot be accurately
tested without risking our own production environment. We may revisit
this again some day by making a separate special domain account for
testing.

@martinisoft martinisoft requested a review from onlyhavecans Aug 6, 2018

@martinisoft

This comment has been minimized.

Member

martinisoft commented Aug 6, 2018

Re-requested review. Caught up to master again and CI should go green.

@martinisoft martinisoft self-assigned this Aug 6, 2018

@onlyhavecans

This changeset is large with a lot of different interface breaking changes so it's a bit hard to parse & review for code issues. It looks good to me though.

Since this one change so many resource interfaces how are we going to communicate concisely and easily for everyone who uses this all the things they have to change before they can use 3.x?

Maybe the Deprecation Warning section should have a tl;dr quick-upgrade checklist that enumerates all the required changes between 12.x & 13.x?

@martinisoft

This comment has been minimized.

Member

martinisoft commented Aug 7, 2018

Since this one change so many resource interfaces how are we going to communicate concisely and easily for everyone who uses this all the things they have to change before they can use 3.x?

Should I do a small release before this to add log warnings about the changes here?

Maybe the Deprecation Warning section should have a tl;dr quick-upgrade checklist that enumerates all the required changes between 12.x & 13.x?

I have done this in bullet point form in a more concise setup.

@onlyhavecans

This comment has been minimized.

Contributor

onlyhavecans commented Aug 7, 2018

I think having a release that warns of the change is a very good idea since this will be a run breaker as is. If you could merge some of the non-interface breaking changes into a minor that has the warnings it would be helpful.

@martinisoft

This comment has been minimized.

Member

martinisoft commented Aug 7, 2018

I think having a release that warns of the change is a very good idea since this will be a run breaker as is. If you could merge some of the non-interface breaking changes into a minor that has the warnings it would be helpful.

I have put up #59 to do this. Once it's merged I can remove those changed from this PR which will cut down a bit on the noise of the changes.

@martinisoft martinisoft requested a review from onlyhavecans Aug 7, 2018

@martinisoft martinisoft merged commit ffc9a62 into master Aug 8, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@martinisoft martinisoft deleted the enhancement/upgrade_chef_13 branch Aug 8, 2018

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