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

Add the steps to setup gbq integration testing to the contributing docs #14144

Conversation

parthea
Copy link
Contributor

@parthea parthea commented Sep 3, 2016

Pull requests from forked repositories cannot access the existing secure Google BigQuery credentials which are on Travis at pydata/pandas. In order for contributors to run Google BigQuery integration tests on Travis, they need to create a pull request on their forked repository of pandas with their own secure Google BigQuery credentials.

I've updated the contributing documentation to include the steps required to run Google BigQuery integration tests on a forked repository of pandas.

@parthea parthea force-pushed the add-gbq-integration-test-setup-to-docs branch 2 times, most recently from d202284 to 2628574 Compare September 3, 2016 13:52
@parthea parthea force-pushed the add-gbq-integration-test-setup-to-docs branch 3 times, most recently from 063c454 to a99dd67 Compare September 3, 2016 14:01
@jreback
Copy link
Contributor

jreback commented Sep 3, 2016

any easier way to do this?

@parthea parthea force-pushed the add-gbq-integration-test-setup-to-docs branch from a99dd67 to f5d4c0f Compare September 3, 2016 14:07
@codecov-io
Copy link

codecov-io commented Sep 3, 2016

Current coverage is 85.25% (diff: 100%)

Merging #14144 into master will increase coverage by <.01%

@@             master     #14144   diff @@
==========================================
  Files           139        139          
  Lines         50496      50502     +6   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          43049      43058     +9   
+ Misses         7447       7444     -3   
  Partials          0          0          

Powered by Codecov. Last update 8023029...6383bba

@parthea
Copy link
Contributor Author

parthea commented Sep 3, 2016

any easier way to do this?

The easiest way is to use the existing encrypted credentials that are already on Travis, but we would need to create a branch from master each time. If we don't use existing credentials, then we need to ask contributors to create their own encrypted credentials. Should I consolidate text and reduce the number of steps? (For example, assume that users already know how to create BigQuery credentials in JSON format?)

@parthea parthea force-pushed the add-gbq-integration-test-setup-to-docs branch from f5d4c0f to 5c5541e Compare September 3, 2016 14:13
@jreback
Copy link
Contributor

jreback commented Sep 3, 2016

The easiest way is to use the existing encrypted credentials that are already on Travis, but we would need to create a branch from master each time.

what does this mean? most branches are forks of master

@parthea
Copy link
Contributor Author

parthea commented Sep 3, 2016

Sorry that I wasn't clear. I meant to say that we would need to create a branch from pydata/pandas which is not on a fork. The credentials can only be encrypted/decrypted from the same repo (not fork).

For example:
The credentials can be decrypted when the PR shows:
parthea wants to merge 2 commits into master from enable-gbq-unit-tests
but cannot be decrypted when the PR shows:
parthea wants to merge 2 commits into pydata:master from parthea:enable-gbq-unit-tests

@jreback
Copy link
Contributor

jreback commented Sep 3, 2016

ok I c. another (maybe easier option), is to have the travis secure key embedded in a file, then we could have a script which a user could run to take there credentials and update the encoded credentials file and the key very easily. (and of course make sure to exclude them from being added to the repo).

@jreback
Copy link
Contributor

jreback commented Sep 3, 2016

is this the 'name' of the secure key that we have? "$encrypted_1d9d7b1f171b_iv"

@parthea
Copy link
Contributor Author

parthea commented Sep 3, 2016

Yes, that is the name of the environment variables encrypted_1d9d7b1f171b_iv and encrypted_1d9d7b1f171b_key which contain the private iv and key, which is only available on the travis repo that encrypted the file. Each repo has a different environment variable.

@parthea
Copy link
Contributor Author

parthea commented Sep 3, 2016

You're right. It makes sense to automate these steps. I'll put together a script in order to reduce the steps.

@jorisvandenbossche
Copy link
Member

@parthea Good to document this!

Some general comments:

  • can you clarify that we only expect to do this from contributors if they are doing a PR related to the gbq module
  • I would make it a separate section (and maybe put it at the end), and then refer to this section from the sections on running tests

@jreback
Copy link
Contributor

jreback commented Sep 4, 2016

this is not mergeable ATM / see above comments

@parthea parthea force-pushed the add-gbq-integration-test-setup-to-docs branch 4 times, most recently from 3fdaa6d to df1eb02 Compare September 4, 2016 17:08
@parthea parthea force-pushed the add-gbq-integration-test-setup-to-docs branch 3 times, most recently from a6448c3 to 2a9a830 Compare September 5, 2016 16:00
read -d "\n" TRAVIS_KEY TRAVIS_IV <<<$(travis encrypt-file $GBQ_JSON_FILE \
travis_gbq.json.enc -f | grep -o "\w*_iv\|\w*_key");

echo "Adding your secure key to .travis.yml ..."
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to move this check OUT of .travis.yml and actually put in a separate (yaml or txt) type of file, then you don't have to do all of this crazy sub/replace stuff, just have a file like

TRAVIS_IV=....
GBQ_PROJECT_ID=...

then simply re-write the file

then .travis.yml section is:

ci\travis_process_gbq_encryption.sh

ci\travis_process_gbq_encryption.sh

# read the variables / project id from the file
 - if [ -n '$encrypted_f44557bb53ea_iv' ]; then
         openssl aes-256-cbc -K $encrypted_f44557bb53ea_key
         -iv $encrypted_f44557bb53ea_iv -in ci/travis_gbq.json.enc
          -out ci/travis_gbq.json -d;
         export GBQ_PROJECT_ID='pandas-140401';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@parthea parthea force-pushed the add-gbq-integration-test-setup-to-docs branch from 2a9a830 to 731bad3 Compare September 5, 2016 18:49
@parthea parthea force-pushed the add-gbq-integration-test-setup-to-docs branch 5 times, most recently from 6383bba to bb2e2b6 Compare September 6, 2016 01:42
@parthea
Copy link
Contributor Author

parthea commented Sep 6, 2016

@jreback @jorisvandenbossche All gbq tests passed at parthea#3. I've moved gbq integration testing to a separate section in the contributing docs. Please take a look.

@jreback jreback added this to the 0.19.0 milestone Sep 6, 2016
@jreback
Copy link
Contributor

jreback commented Sep 6, 2016

lgtm.

tests are `encrypted <https://docs.travis-ci.com/user/encrypting-files/>`__
on Travis-CI and are only accessible from the pydata/pandas repository. In
addition to creating a pull request on pydata/pandas, contributors should also
create a pull request on their forked repository and add encrypted credentials
Copy link
Member

Choose a reason for hiding this comment

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

Is it needed to create a PR on your fork?

Copy link
Contributor Author

@parthea parthea Sep 7, 2016

Choose a reason for hiding this comment

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

@jorisvandenbossche No, it is not needed. Thanks for catching that! I've updated the instructions. Please take another look.

@parthea parthea force-pushed the add-gbq-integration-test-setup-to-docs branch from bb2e2b6 to 79f790a Compare September 7, 2016 02:51

#. Create a new branch from the branch used in your pull request. Commit the
encrypted file called ``travis_gbq.json.enc`` as well as the file
``travis_gbq_config.txt``, in an otherwise empty commit. DO NOT commit the
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this file already committed? Or will it change due to the script you call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct. The file is already committed. Both travis_gbq.json.enc and travis_gbq_config.txt will change when users run ``/travis_encrypt_gbq.sh...` so we need to tell users to commit those files (or changes) in a new branch.

Copy link
Member

Choose a reason for hiding this comment

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

ok!

@jorisvandenbossche
Copy link
Member

Small further question, otherwise looks good!

@parthea
Copy link
Contributor Author

parthea commented Sep 7, 2016

Here is a link to the build from my branch which has my encrypted credentials so that gbq test are not skipped. https://travis-ci.org/parthea/pandas/jobs/158060278

@jorisvandenbossche jorisvandenbossche merged commit 9b7efd6 into pandas-dev:master Sep 7, 2016
@jorisvandenbossche
Copy link
Member

@parthea Nice, thanks a lot!

@parthea parthea deleted the add-gbq-integration-test-setup-to-docs branch September 7, 2016 13:23
trbs added a commit to trbs/pandas that referenced this pull request Sep 12, 2016
…eter

* github.com:pydata/pandas: (554 commits)
  BUG: compat with Stata ver 111
  Fix: F999 dictionary key '2000q4' repeated with different values (pandas-dev#14198)
  BLD: Test for Python 3.5 with C locale
  BUG: DatetimeTZBlock can't assign values near dst boundary
  BUG: union_categorical with Series and cat idx
  BUG: fix str.contains for series containing only nan values
  BUG: Categorical constructor not idempotent with ext dtype
  TST: Make encoded sep check more locale sensitive (pandas-dev#14161)
  DOC: minor typo in 0.19.0 whatsnew file (pandas-dev#14185)
  BUG: fix tz-aware datetime convert to DatetimeIndex (GH 14088)
  BUG : bug in setting a slice of a Series with a np.timedelta64
  RLS: v0.19.0rc1
  DOC: clean-up 0.19.0 whatsnew file (pandas-dev#14176)
  DOC: cleanup build warnings (pandas-dev#14172)
  Add steps to run gbq integration testing to the contributing docs (pandas-dev#14144)
  ENH: concat and append now can handle unordered categories (pandas-dev#13767)
  DEPR: Deprecate pandas.core.datetools (pandas-dev#14105)
  API/DEPR: Remove +/- as setops for DatetimeIndex/PeriodIndex (GH9630) (pandas-dev#14164)
  Fix trivial typo in comment (pandas-dev#14174)
  API/DEPR: Remove +/- as setops for Index (GH8227) (pandas-dev#14127)
  ...
trbs added a commit to trbs/pandas that referenced this pull request Sep 12, 2016
* github.com:pydata/pandas: (554 commits)
  BUG: compat with Stata ver 111
  Fix: F999 dictionary key '2000q4' repeated with different values (pandas-dev#14198)
  BLD: Test for Python 3.5 with C locale
  BUG: DatetimeTZBlock can't assign values near dst boundary
  BUG: union_categorical with Series and cat idx
  BUG: fix str.contains for series containing only nan values
  BUG: Categorical constructor not idempotent with ext dtype
  TST: Make encoded sep check more locale sensitive (pandas-dev#14161)
  DOC: minor typo in 0.19.0 whatsnew file (pandas-dev#14185)
  BUG: fix tz-aware datetime convert to DatetimeIndex (GH 14088)
  BUG : bug in setting a slice of a Series with a np.timedelta64
  RLS: v0.19.0rc1
  DOC: clean-up 0.19.0 whatsnew file (pandas-dev#14176)
  DOC: cleanup build warnings (pandas-dev#14172)
  Add steps to run gbq integration testing to the contributing docs (pandas-dev#14144)
  ENH: concat and append now can handle unordered categories (pandas-dev#13767)
  DEPR: Deprecate pandas.core.datetools (pandas-dev#14105)
  API/DEPR: Remove +/- as setops for DatetimeIndex/PeriodIndex (GH9630) (pandas-dev#14164)
  Fix trivial typo in comment (pandas-dev#14174)
  API/DEPR: Remove +/- as setops for Index (GH8227) (pandas-dev#14127)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants