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

(FM-7597) RSAPI Transport register function and Schema validation #141

Closed
wants to merge 1 commit into from
Closed

(FM-7597) RSAPI Transport register function and Schema validation #141

wants to merge 1 commit into from

Conversation

da-ar
Copy link

@da-ar da-ar commented Dec 7, 2018

No description provided.

@da-ar da-ar requested a review from DavidS December 7, 2018 15:05
@da-ar da-ar changed the base branch from master to transport December 7, 2018 15:27
Copy link
Contributor

@DavidS DavidS left a comment

Choose a reason for hiding this comment

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

if the validation changes were in a separate commit we could pull them directly into master

this PR needs to be retargeted against the (new) transport feature branch

raise Puppet::DevError, 'TransportSchemaDef requires definition to be a Hash' unless definition.is_a?(Hash)
super(definition, :connection_info)
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Given how small TransportSchemaDef and TypeDefinition have become I'd pref to have them all in one file:

module Puppet::ResourceApi
  # pre-declare class
  class BaseTypeDefinition; end
 
  class Puppet::ResourceApi::TypeDefinition < Puppet::ResourceApi::BaseTypeDefinition
    # ...
  ...
  ..
  class BaseTypeDefinition
    # the rest

# Provides accessor methods for the type being provided
class Puppet::ResourceApi::TransportSchemaDef < Puppet::ResourceApi::BaseTypeDefinition
def initialize(definition)
raise Puppet::DevError, 'TransportSchemaDef requires definition to be a Hash' unless definition.is_a?(Hash)
Copy link
Contributor

Choose a reason for hiding this comment

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

duplicate check - if you're doing this because of the class name in the error message, use self.class.name in the base class.

# Remote target transport API
module Puppet::ResourceApi::Transport
class << self
attr_reader :transports
Copy link
Contributor

Choose a reason for hiding this comment

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

exposing the transports like this will allow folks to directly interact with the hash and add/modify/delete entries without validation. there is no need for the accessor yet.

@DavidS DavidS closed this Dec 12, 2018
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.

2 participants