Phase 1 Update get_bucket to take Bucket object within Storage Client

[lbristol88]

Part of issue: #7762

@tswast
@engelke

[tswast]

LGTM once the example is updated.

[tswast]

I fear that this example could lead to people thinking they have to create a Bucket object when they can just pass in a string.

This implementation is more to provide the same mechanism as reload() is currently used, so we should indicate that somehow you got a bucket but think it might be out of date and want to reload it.

Maybe change this line to get_bucket("my-bucket-name"), and then between this and the next call to get_bucket, add a comment like # Time passes. Another program may have modified the bucket in the meantime, so you want to get the latest state.

[lbristol88]

Modified the example as directed!

[tswast]

System test failure appears to be unrelated to this change.

______________ TestRetentionPolicy.test_bucket_w_retention_period ______________
self = <tests.system.TestRetentionPolicy testMethod=test_bucket_w_retention_period>
    def test_bucket_w_retention_period(self):
        import datetime
        from google.api_core import exceptions
        period_secs = 10
        new_bucket_name = "w-retention-period" + unique_resource_id("-")
        bucket = retry_429(Config.CLIENT.create_bucket)(new_bucket_name)
        self.case_buckets_to_delete.append(new_bucket_name)
        bucket.retention_period = period_secs
        bucket.default_event_based_hold = False
        bucket.patch()
        self.assertEqual(bucket.retention_period, period_secs)
        self.assertIsInstance(bucket.retention_policy_effective_time, datetime.datetime)
        self.assertFalse(bucket.default_event_based_hold)
        self.assertFalse(bucket.retention_policy_locked)
        blob_name = "test-blob"
        payload = b"DEADBEEF"
        blob = bucket.blob(blob_name)
        blob.upload_from_string(payload)
        other = bucket.get_blob(blob_name)
        self.assertFalse(other.event_based_hold)
        self.assertFalse(other.temporary_hold)
        self.assertIsInstance(other.retention_expiration_time, datetime.datetime)
        with self.assertRaises(exceptions.Forbidden):
            other.delete()
        bucket.retention_period = None
        bucket.patch()
        self.assertIsNone(bucket.retention_period)
        self.assertIsNone(bucket.retention_policy_effective_time)
        self.assertFalse(bucket.default_event_based_hold)
        self.assertFalse(bucket.retention_policy_locked)
        other.reload()
        self.assertFalse(other.event_based_hold)
        self.assertFalse(other.temporary_hold)
>       self.assertIsNone(other.retention_expiration_time)
E       AssertionError: datetime.datetime(2019, 5, 6, 19, 57, 0, 946000, tzinfo=<UTC>) is not None
tests/system.py:1506: AssertionError
1 failed, 54 passed, 11 skipped, 1 error in 146.87 seconds
Command py.test --quiet tests/system.py failed with exit code 1
Session system-2.7 failed.

[bboe]

Hi, we just noticed this change updating from a minor version as our code was doing bucket(bucket_name="ourbucket") and with this minor version change things broke. I'm curious, does this package follow any versioning guidelines, such as semantic versioning? We just want to know how flexible we can be with our version dependencies. Thanks!

[crwilcox]

Hi @bboe ,

We do our best to conform to semver, but occasionally mistakes are made. This is one of those times. This change, particularly this line https://github.com/googleapis/google-cloud-python/pull/7856/files#diff-dbb49bf7288430753b6267b849bd01bbR205 altered the argument name which broke your code :(

Sorry for the randomization. Ideally this wouldn't happen, and thank you for pointing this out. This was an oversight on this change to consider this minor.