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

Refactor XML handling of {set,get,delete}_bucket_notification APIs #990

Conversation

balamurugana
Copy link
Member

@balamurugana balamurugana commented Oct 5, 2020

No description provided.

@balamurugana balamurugana force-pushed the Refactor-XML-handling-of-set-get-delete_bucket_notification-APIs branch 2 times, most recently from 35fed13 to 052995e Compare October 5, 2020 04:07
@balamurugana balamurugana force-pushed the Refactor-XML-handling-of-set-get-delete_bucket_notification-APIs branch 2 times, most recently from 65e544d to 893a00b Compare October 5, 2020 06:16
@balamurugana balamurugana force-pushed the Refactor-XML-handling-of-set-get-delete_bucket_notification-APIs branch from 893a00b to 394e63f Compare October 13, 2020 16:05
@balamurugana balamurugana force-pushed the Refactor-XML-handling-of-set-get-delete_bucket_notification-APIs branch from 394e63f to 52afa5d Compare October 15, 2020 21:30
kannappanr
kannappanr previously approved these changes Oct 19, 2020
@balamurugana balamurugana force-pushed the Refactor-XML-handling-of-set-get-delete_bucket_notification-APIs branch from 52afa5d to cb286fa Compare October 19, 2020 10:39
kannappanr
kannappanr previously approved these changes Oct 19, 2020
Copy link
Collaborator

@kannappanr kannappanr left a comment

Choose a reason for hiding this comment

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

LGTM

@balamurugana balamurugana force-pushed the Refactor-XML-handling-of-set-get-delete_bucket_notification-APIs branch 3 times, most recently from 6449272 to 3213720 Compare October 19, 2020 18:43
kannappanr
kannappanr previously approved these changes Oct 19, 2020
@balamurugana balamurugana force-pushed the Refactor-XML-handling-of-set-get-delete_bucket_notification-APIs branch from 3213720 to 55bf8a4 Compare October 20, 2020 05:29
Copy link
Collaborator

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

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

@balamurugana ,

I ran a set_bucket_notification test on my setup, and hit a TypeError:

TypeError: SubElement() argument 1 must be xml.etree.ElementTree.Element, not QueueConfig

My MinIO server is the usual stand-alone minio server, started like:
minio server <data-location>

Then I set the Redis notify configuration parameters and rebooted the minio server.

Here is the mc command, I used to set the notify configuration parameters:

$ mc admin config set myminio/ notify_redis:1 address="127.0.0.1:6379" format="namespace" key="bucketevents" password="yoursecret" queue_dir="" queue_limit="0"
Setting new key has been successful.
Please restart your server with `mc admin service restart myminio/`.

Here is the test script:

from minio import Minio
from minio.notificationconfig import (NotificationConfig, PrefixFilterRule,
                                      QueueConfig)

client = Minio(
    "localhost:9000",
    access_key="minio",
    secret_key="minio123",
    secure=False,
)

config = NotificationConfig(
    queue_config_list=[
        QueueConfig(
            "arn:minio:sqs:us-east-1:1:redis",
            ['s3:ObjectCreated:*'],
            config_id="1",
            prefix_filter_rule=PrefixFilterRule("abc"),
        ),
    ],
)

client.set_bucket_notification("bucket", config)

Running the above script throws the following error messages:

$ python3.8 run_set_bucket_notification.py 
Traceback (most recent call last):
  File "run_set_bucket_notification.py", line 47, in <module>
    client.set_bucket_notification("bucket", config)
  File "/home/ersan/work/src/github.com/minio/minio-py/minio/api.py", line 753, in set_bucket_notification
    body = marshal(config)
  File "/home/ersan/work/src/github.com/minio/minio-py/minio/xml.py", line 100, in marshal
    return getbytes(obj.toxml(None))
  File "/home/ersan/work/src/github.com/minio/minio-py/minio/notificationconfig.py", line 313, in toxml
    config.toxml(config)
  File "/home/ersan/work/src/github.com/minio/minio-py/minio/notificationconfig.py", line 219, in toxml
    element = SubElement(element, "QueueConfiguration")
  File "/home/ersan/work/src/github.com/minio/minio-py/minio/xml.py", line 35, in SubElement
    element = ET.SubElement(parent, tag)
TypeError: SubElement() argument 1 must be xml.etree.ElementTree.Element, not QueueConfig

Please let me know if you would like to make some changes and need me to run the test.

@balamurugana balamurugana force-pushed the Refactor-XML-handling-of-set-get-delete_bucket_notification-APIs branch from 55bf8a4 to f7f395d Compare October 20, 2020 08:31
@balamurugana
Copy link
Member Author

@ebozduman fixed and updated this PR

@ebozduman
Copy link
Collaborator

Great! Testing it again after sync

Copy link
Collaborator

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

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

Now, get_bucket_notification api is having another type issue:

$ python3.8 run_get_bucket_notification.py 
Traceback (most recent call last):
  File "run_get_bucket_notification.py", line 33, in <module>
    config = client.get_bucket_notification("bucket")
  File "/home/ersan/work/src/github.com/minio/minio-py/minio/api.py", line 728, in get_bucket_notification
    return unmarshal(NotificationConfig, response.data.decode())
  File "/home/ersan/work/src/github.com/minio/minio-py/minio/xml.py", line 85, in unmarshal
    return cls.fromxml(ET.fromstring(xmlstring))
  File "/home/ersan/work/src/github.com/minio/minio-py/minio/notificationconfig.py", line 298, in fromxml
    queue_config_list.append(QueueConfig.fromxml(tag))
  File "/home/ersan/work/src/github.com/minio/minio-py/minio/notificationconfig.py", line 208, in fromxml
    suffix_filter_rule) = cls.parsexml(element)
  File "/home/ersan/work/src/github.com/minio/minio-py/minio/notificationconfig.py", line 125, in parsexml
    if filter_rule.name() == "prefix":
TypeError: 'str' object is not callable

@balamurugana balamurugana force-pushed the Refactor-XML-handling-of-set-get-delete_bucket_notification-APIs branch from f7f395d to 629ca81 Compare October 20, 2020 08:57
@balamurugana
Copy link
Member Author

@ebozduman

Now, get_bucket_notification api is having another type issue:

Fixed and updated the PR

@ebozduman
Copy link
Collaborator

By the way, here is the script I use to get bucket notification configuration, if any:

from minio import Minio

client = Minio("localhost:9000",
    access_key="minio",
    secret_key="minio123",
    secure=False,
)

config = client.get_bucket_notification("bucket")
print(vars(config))

@balamurugana balamurugana force-pushed the Refactor-XML-handling-of-set-get-delete_bucket_notification-APIs branch from 629ca81 to f08882e Compare October 20, 2020 09:21
@balamurugana
Copy link
Member Author

By the way, here is the script I use to get bucket notification configuration, if any:

@ebozduman There was a problem in naming unit test file caused the tests wasn't kicked in. I fixed that as well.

Copy link
Collaborator

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

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

Tests are successfully completed.
As notification is triggered (creation of objects under bucket, "bucket", and with the name prefix "abc"), I see data coming to redis client and redis server.
LGTM

@balamurugana balamurugana merged commit de50dd4 into minio:master Oct 20, 2020
@balamurugana balamurugana deleted the Refactor-XML-handling-of-set-get-delete_bucket_notification-APIs branch October 20, 2020 14:31
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

4 participants