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

Bigtable: create snippets.py for python docs. #6012

Merged
merged 17 commits into from
Oct 29, 2018

Conversation

sangramql
Copy link
Contributor

No description provided.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 18, 2018
@tseaver tseaver added documentation api: bigtable Issues related to the Bigtable API. labels Sep 18, 2018
Copy link
Contributor

@tseaver tseaver left a comment

Choose a reason for hiding this comment

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

We need to add support for testing the snippets. See bigquery/nox.py for an example.

@tseaver tseaver removed the request for review from dhermes September 18, 2018 22:03
@tseaver tseaver changed the title Create snippets.py for python docs. Bigtable: create snippets.py for python docs. Sep 18, 2018
Copy link
Contributor

@tseaver tseaver left a comment

Choose a reason for hiding this comment

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

Are you actually running the snippets yourself? They don't pass for me.

bigtable/nox.py Outdated
@nox.session
def lint(session):
"""Run linters.

This comment was marked as spam.

This comment was marked as spam.

client = bigtable.Client(project='my-project', admin=True)
for example in _find_examples():
to_delete = []
print '%-25s: %s' % _name_and_doc(example)

This comment was marked as spam.

This comment was marked as spam.

Copy link
Contributor

@tseaver tseaver left a comment

Choose a reason for hiding this comment

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

OK, the changes today look right to me. However, I'm pretty sure that they won't pass. Have you run nox -e snippets to check?

@sduskis sduskis removed the request for review from theacodes October 8, 2018 18:04
@sduskis
Copy link
Contributor

sduskis commented Oct 15, 2018

@sumit-ql, can you please update bigtable/nox.py?

@tseaver
Copy link
Contributor

tseaver commented Oct 15, 2018

@sduskis

@sumit-ql, can you please update bigtable/nox.py?

After #6175, the new location is bigtable/noxfile.py.

@sangramql
Copy link
Contributor Author

Working on fixing the tests, there are dependency failures.

@tseaver
Copy link
Contributor

tseaver commented Oct 18, 2018

@sangramql The file nox.py has been replaced with noxfile.py. To resolve conflicts, please merge the current master into your branch, and re-apply the snippets session there.

You will need to update Nox via:

$ /path/to/venv/bin/pip uninstall nox-automation
$ /path/to/venv/bin/pip install nox

@JustinBeckwith JustinBeckwith added type: docs Improvement to the documentation for an API. and removed documentation labels Oct 18, 2018
bigtable/nox.py Outdated

@nox.session
@nox.parametrize('py', ['2.7', '3.6'])
@nox.parametrize('py', ['2.7'])

This comment was marked as spam.


@pytest.fixture(scope='module')
def client():
return bigtable.Client(project='my-project', admin=True)
return bigtable.Client(project='grass-clump-479', admin=True)

This comment was marked as spam.

@@ -47,6 +51,7 @@ def to_delete():
item.delete()


@pytest.mark.order1

This comment was marked as spam.

@sangramql
Copy link
Contributor Author

Hi @tseaver , Fixed the tests, could you please review it.

@sduskis
Copy link
Contributor

sduskis commented Oct 24, 2018

@sumit-ql, it looks like there's a CircleCI failure:

sphinx.errors.SphinxWarning: /var/code/gcp/docs/index.rst:1:toctree contains reference to nonexisting document 'bigtable/index'

Warning, treated as error:
/var/code/gcp/docs/index.rst:1:toctree contains reference to nonexisting document 'bigtable/index'

@sangramql
Copy link
Contributor Author

Hi @tseaver , Fixed the tests, could you please review it.

@sangramql
Copy link
Contributor Author

@tseaver can you suggest the help on this failure. index link is failing for some files, but looks like passing for some. Do we need to change all the links?

@tseaver
Copy link
Contributor

tseaver commented Oct 24, 2018

@sangramql Since PR #6014, docs/bigtable has been a symlink to bigtable/docs, which does include index.rst. Maybe you need to rebase your create_snippet branch on the current master?

@sangramql
Copy link
Contributor Author

@tseaver, my bad, i removed the symlink while submitting the code, can you please review the code.
@AVaksman thank you for all your help in fixing the tests, can you also review the submitted code.

@tseaver tseaver added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 25, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 25, 2018
PRODUCTION = enums.Instance.Type.PRODUCTION
SERVER_NODES = 3
STORAGE_TYPE = enums.StorageType.SSD
LABEL_KEY = u'python-system'

This comment was marked as spam.


# Assuming that there is an existing instance with `INSTANCE_ID`
# on the server already.
# to create an instance see 'link'

This comment was marked as spam.

operation.result(timeout=100)
# [END bigtable_create_cluster]
cluster2 = instance.cluster(cluster_id)
assert cluster2.exists()

This comment was marked as spam.

assert instances_list.__len__() is not 0


def test_bigtable_list_clusters():

This comment was marked as spam.

bigtable/docs/snippets.py Show resolved Hide resolved
client = Client(admin=True)
instance = client.instance(INSTANCE_ID)
(clusters_list, failed_locations_list) = instance.list_clusters()
# [END bigtable_list_clusters]

This comment was marked as spam.

For example:

.. literalinclude:: snippets.py
:start-after: [START bigtable_list_clusters]

This comment was marked as spam.


.. literalinclude:: snippets.py
:start-after: [START bigtable_list_clusters]
:end-before: [END bigtable_list_clusters]

This comment was marked as spam.

For example:

.. literalinclude:: snippets.py
:start-after: [START bigtable_list_clusters]

This comment was marked as spam.


.. literalinclude:: snippets.py
:start-after: [START bigtable_list_clusters]
:end-before: [END bigtable_list_clusters]

This comment was marked as spam.

@AVaksman
Copy link
Contributor

AVaksman commented Oct 25, 2018

@sangramql Please also add snippets for methods,

  • app_profile
  • list_app_profiles
  • get_iam_policy
  • set_iam_policy
  • test_iam_permissions
    to complete examplese for client'.py, cluster.py and instance.py

@tseaver
Copy link
Contributor

tseaver commented Oct 25, 2018

Unrelated Bigtable lint error, fixed in #6309.

Similar unrelated Firestore lint error (why is it even running?), which will be fixed via #6291.

@tseaver
Copy link
Contributor

tseaver commented Oct 25, 2018

@sangramql Thanks for sticking with this PR! If you can address the suggestions / requested new snippets from @AVaksman, I think we'll be ready to merge.

@sangramql
Copy link
Contributor Author

@tseaver, @AVaksman ,
I have fixed the review comments, requesting to please review.
I was not able to add get_iam_policy and set_iam_policy as those test failure due to insufficient permissions. If needed I can add to snippets in separate PR, as those already have code in documentations as of now.

@tseaver
Copy link
Contributor

tseaver commented Oct 29, 2018

@sangramql I'm pretty sure that if you can create an instance, you should be able to call get_iam_policy / set_iam_policy / test_iam_permissions on it.

The only other missing snippet that I see requested by @AVaksman is for instance.update.

@sangramql
Copy link
Contributor Author

@tseaver , @AVaksman Please review the updated code with all the reviews implemented.

@tseaver
Copy link
Contributor

tseaver commented Oct 29, 2018

@sangramql we still need snippets for instance.update and instance.test_iam_permissions.

@sangramql
Copy link
Contributor Author

@tseaver , @AVaksman added the snippet for get and set iam policy.
Please review.

@sangramql
Copy link
Contributor Author

sangramql commented Oct 29, 2018

@sangramql we still need snippets for instance.update and instance.test_iam_permissions.

Added snippets for both the methods. update line 258 and iam_permissions line 360.

@tseaver tseaver merged commit a7d8058 into googleapis:master Oct 29, 2018
@sangramql sangramql deleted the create_snippet branch October 30, 2018 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the Bigtable API. cla: yes This human has signed the Contributor License Agreement. type: docs Improvement to the documentation for an API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants