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

Support for Cloud Tasks #24

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

msdinit
Copy link

@msdinit msdinit commented Aug 6, 2020

Hi!
Hope you're accepting pull requests with new integrations.

This is an integration to Cloud Tasks, pretty minimal for now - it allow getting queue by name and creating tasks.
I tried to follow your conventions, to the best of my understanding, but do tell me if you prefer something changed!

One quirk I bumped into is the requirement for custom routing metadata here and here - could not find this anywhere in the docs, so I basically followed what Python implementation did.

I'm also not sure how to go about testing - I confirmed that I can fetch queues and create new tasks manually, but I assume (looking at tests) that adding tests requires you to enable additional services on your test project?

In any case, would be glad to hear back from you.
(On an unrelated note, fresh fetch of https://github.com/googleapis/googleapis is incredibly slow, I wonder if cargo has no optimizations for submodules and re-fetches it whole for each update of main repo)

Copy link
Member

@Hirevo Hirevo left a comment

Choose a reason for hiding this comment

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

Hi !

Thank you for contributing, we're indeed happily accepting pull requests for new service integrations, because, in my personal case, I'm only using a small fraction of everything GCP has to offer, so it would be hard for me to build all these integrations on my own (and I think people with an actual use-case for an integration are valuable to help improve them).

I have never used Cloud Tasks before, and I am currently reading through its documentation, so I don't know if I'll be able to help you much on the API design of it right now.
Regarding the actual implementation code, I find it very well done, it's nicely organized and with a bunch of comments making it easy to read through. Thank you for this. 👍
I pretty much don't have anything to complain about in it, just a single minor suggestion.

As for the tests, yes, we use the GCP project of the company I am working at for the CI, and we pass the secrets (like the credentials) through environment variables, so they are writeable independently of any specific GCP project.
It seems at first glance that Cloud Tasks is pretty inexpensive (the 1st million tasks are apparently free), so setting it up seems that it won't cause any problem.
The thing is that I personally don't know what flow is interesting to test (how the service is typically used).

@Hirevo
Copy link
Member

Hirevo commented Aug 8, 2020

Regarding the cloning speed, I can only agree with you, it is very painful.
It seems that the googleapis/googleapis's latest revision isn't large at all on its own, it's probably due to a very large Git history (very large diffs) or large files having been pushed in that repository and then removed at some later point but now saved in their .git.
We should most-likely consider vendoring the protobuf files directly (along with their license notice, of course).

@Hirevo Hirevo changed the title Feature / Cloud Tasks Support for Cloud Tasks Aug 8, 2020
@msdinit
Copy link
Author

msdinit commented Aug 10, 2020

Thanks for the review!
I renamed the method as you suggested, and added 2 new ones - get_task on Queue object and delete_task on the Task object, mainly for testing.

I also added an example test in tests/task.rs, but commented it out for now, so you could enable it when you see fit.
I think the most common use-case for Tasks is to create a task and not do with it anything else, but I added the get and delete for ease of testing - creating a task and not deleting it wouldn't be ideal 😄 . The service account would require roles/cloudtasks.admin role for this. Allowing service account access only to a single queue (as opposed to all queues in the project) isn't well-described anywhere, so I include a command I use just in case you need it (queue can be created from console)

gcloud tasks queues add-iam-policy-binding projects/{project-name}/locations/{location-id}/queues/{queue-name} --member serviceAccount:{service-account-email} --role roles/cloudtasks.admin

Copy link
Member

@Hirevo Hirevo left a comment

Choose a reason for hiding this comment

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

Sorry for the delayed review.

Thanks for the test case !
I was able to successfully run it, after only one minor change.
The service account I use locally turned out to already have the required permission, but thank you for the tip with the gcloud command.

I think we should be good to uncomment the test case, it should be all right regarding the CI's GCP project.

//
// //? Create test task
// let request_config = tasks::HttpRequestConfig::new("https://example.com")
// .http_method(tasks::HttpMethod::Get)
Copy link
Member

Choose a reason for hiding this comment

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

While running this test, I got the following error from GCP:

InvalidArgument: Body can only be set if the http_method is POST, PUT, or PATCH.

Which makes sense.

The following change fixed this:

Suggested change
// .http_method(tasks::HttpMethod::Get)
// .http_method(tasks::HttpMethod::Post)

// let mut client = assert_ok!(setup_client().await);
//
// //? Get test queue
// let queue = client.queue(env!("GCP_TEST_TOPIC")).await;
Copy link
Member

Choose a reason for hiding this comment

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

I (and the CI) tend to run the tests for all integrations at once.
Since the Pub/Sub integration already uses the GCP_TEST_TOPIC variable, we may not be able to specify different values for the Pub/Sub topic to use and the Tasks queue to use.

Suggested change
// let queue = client.queue(env!("GCP_TEST_TOPIC")).await;
// let queue = client.queue(env!("GCP_TEST_QUEUE")).await;

I think we should eventually move to properly namespace these environment variables, so that we could do this if needed: GCP_TEST_PUBSUB_TOPIC and GCP_TEST_TASKS_TOPIC, but that can be done in a separate PR.

// };
// }
//
// macro_rules! assert_some {
Copy link
Member

Choose a reason for hiding this comment

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

Since this is not used currently, I think we can mark it as #[allow(unused)] to avoid the warning.

Suggested change
// macro_rules! assert_some {
// #[allow(unused)]
// macro_rules! assert_some {

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.

2 participants