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 test for virtual hosted bucket #1187

Merged
merged 4 commits into from Apr 4, 2024

Conversation

shundezhang
Copy link
Contributor

Add integration test for the virtual hosted bucket feature proposed to the upstream ceph-radosgw Juju charm:
https://review.opendev.org/c/openstack/charm-ceph-radosgw/+/904554

shundezhang added a commit to shundezhang/charm-ceph-radosgw that referenced this pull request Feb 29, 2024
Closes-Bug: #1871745
Change-Id: I295baab496d1eb95daaa8073d4119d01b90d0b38

func-test-pr: openstack-charmers/zaza-openstack-tests#1187
@ajkavanagh ajkavanagh requested a review from sabaini March 3, 2024 12:21
Copy link
Contributor

@sabaini sabaini left a comment

Choose a reason for hiding this comment

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

Thank you for this test! Two minor comments inline

# test virtual hosted bucket
logging.info('Testing virtual hosted bucket')
primary_client.Bucket(container_name).Acl().put(ACL='public-read')
primary_client.Object(container_name,obj_name).Acl().put(ACL='public-read')
Copy link
Contributor

Choose a reason for hiding this comment

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

One doubt around this. The above makes the test container publically available. Later on the test checks access for the test container using test credentials. Given that the container is now publically available, would the test credentials even be considered then?

I feel it would be preferable to isolate this test into a test method of it's own (and not share a container name with the other tests), also given that this test method is awfully long already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is moved to test_005_virtual_hosted_bucket function

primary_client.Object(container_name,obj_name).Acl().put(ACL='public-read')
public_hostname = zaza_model.get_application_config(self.primary_rgw_app)["os-public-hostname"]["value"]
url = primary_endpoint + "/" + obj_name
headers = {'host': container_name + "." + public_hostname}
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor style nit: suggest to use f-strings for readability

url = f"{primary_endpoint}/{obj_name}"
headers = {'host': f"{container_name}.{public_hostname}"}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is changed

@shundezhang shundezhang force-pushed the master branch 2 times, most recently from c15006c to 0b8324f Compare March 8, 2024 12:10
@shundezhang shundezhang force-pushed the master branch 7 times, most recently from ea541ea to 83ab504 Compare April 2, 2024 06:38
@ajkavanagh
Copy link
Contributor

@shundezhang hi; it would be really useful if you didn't force push, please. Each time you do, you erase the previous commits which makes it really hard to see what the differences are between your commits, which also slows down the review process. We can squash small edits down as part of the merge. Thanks.

@shundezhang
Copy link
Contributor Author

@ajkavanagh sure will do. Actually it is easier for me to not force push.

@sabaini sabaini merged commit 01ca8f5 into openstack-charmers:master Apr 4, 2024
3 checks passed
shundezhang added a commit to shundezhang/charm-ceph-radosgw that referenced this pull request Apr 5, 2024
func-test-pr: openstack-charmers/zaza-openstack-tests#1187

Closes-Bug: #1871745
Change-Id: I295baab496d1eb95daaa8073d4119d01b90d0b38
openstack-mirroring pushed a commit to openstack/openstack that referenced this pull request Apr 5, 2024
* Update charm-ceph-radosgw from branch 'master'
  to 6f2a7540e8102fc7575731e7e059695cd1c28a76
  - Add a config option for virtual hosted bucket
    
    func-test-pr: openstack-charmers/zaza-openstack-tests#1187
    
    Closes-Bug: #1871745
    Change-Id: I295baab496d1eb95daaa8073d4119d01b90d0b38
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.

None yet

3 participants