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

Payment data sources #34

Merged
merged 4 commits into from
Jun 5, 2018
Merged

Conversation

gaetanfl
Copy link
Contributor

In order to make payment for vps resource creation or any other kind of classic (non cloud) ovh products a data source for payments method would make things easier.

I've a question regarding tests, if I use two env variable with filter patterns for credit card or bank account and check there is a match, is it enough ?

@tombuildsstuff
Copy link
Contributor

hey @gaetanfl

Thanks for this PR :)

Taking a quick look at this PR it contains sensitive information (IBAN / Credit Card Numbers) which aren't encrypted in the state. Since this is the first time this kind of information has been exposed across providers - we need to chat about this internally before proceeding - please bear with us for the moment and we'll get back to you shortly.

Thanks!

@gaetanfl
Copy link
Contributor Author

gaetanfl commented May 22, 2018

Hi,
in fact IBAN and credit card numbers are truncated you have just the beginning and the end which shouldn't be a problem.
For credit card, the first 6 digits and last 4 are available and for IBAN First 9 digits and last 4.

@yanndegat
Copy link
Collaborator

hi @tombuildsstuff & @gaetanfl

actually, i'm also checking internally @ovh if this kind of info isnt to sensible to be exposed in a terraform state file.

i'm not so sure we want to take this risk.

@yanndegat
Copy link
Collaborator

hi @gaetanfl

i've checked internally. there should be no problem outputting these values as they are returned by the API
yet, terraform is an infrastructure tool and there are sensitive info not useful to bootstrap infrastructure in the computed attributes.

Do you think we could export only the internal id return by the API if the datasource is found ?
or do you need other attributes ?

@gaetanfl
Copy link
Contributor Author

gaetanfl commented May 22, 2018

I guess we could keep for bank account:

  • Description
  • Default
  • State
  • Id

and for Credit Card:

  • Description
  • Expiration
  • Default
  • State
  • Id

Expiration is to check if we shoudl ask account administrator to add another one

Will that be ok for you ?

@yanndegat
Copy link
Collaborator

thanks a lot

i think this would be better and avoid us further misunderstandings from end users

for expiration date, as it's a credential in a lot forms, couldn't we set a "still_valid_date" filter
that would filter all credit cards that has expired before this date ?

@gaetanfl
Copy link
Contributor Author

I will just remove it, if someone need it it won't be difficult to implement

@jkohrman
Copy link

Hi there @gaetanfl - This is looking much better, thanks!

Quick question for you since I'm not familiar with this particular OVH API. What is the expected value of the State attribute?

@@ -46,6 +46,8 @@ func Provider() terraform.ResourceProvider {
// New naming schema (issue #23)
"ovh_cloud_region": dataSourcePublicCloudRegion(),
"ovh_cloud_regions": dataSourcePublicCloudRegions(),
"ovh_credit_card": dataSourceCreditCard(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we map the ovh api by prefixing datasources by "ovh_me_" ?
pls ? and rename the go functions accordingly ?

thanks a lot

@yanndegat
Copy link
Collaborator

hi @jkohrman
state is valid/errored enums based on ovh info regarding the credit card.
do you think this attribute is sensitive ?

@yanndegat yanndegat merged commit bd505b6 into ovh:master Jun 5, 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.

4 participants