google-apis-rs / google-cloud-rs

Asynchronous Rust bindings for Google Cloud Platform APIs.
176 stars 48 forks source link

Refactor build.rs to reduce duplicate code #11

Closed mwilliammyers closed 4 years ago

mwilliammyers commented 4 years ago

Hopefully this will make it easier to add new APIs going forward.

In the future it might be nice to explore using the artman/YAML config files or otherwise automatically determine the list of proto files.

Hirevo commented 4 years ago

I've made some changes to bring the CI more up-to-date and the tests are passing on master but the new CI configuration doesn't seem to be picked by just re-running the jobs of this PR (it just fails to even checkout, now).
You may need to rebase this branch onto the latest master or merge the master branch into this branch.
I don't know if I can do that myself since it is your fork.
In the worst case, we could still merge the PR without the tests passing, see if it passes on master, and fix it in another PR if it still doesn't (at least, the new PR would be using the new CI config).

mwilliammyers commented 4 years ago

Hmmm I rebased my branch on top of master and now it looks like I am getting a real error. I will investigate it, but as far as I can tell I haven't changed the logic in build.rs (except for creating the directory first if it does not exist yet).

Hirevo commented 4 years ago

Sorry for the delay.

It seemed weird to me as well, the PR passed the tests on my machine and after some digging I just found that it's because GitHub Actions actually does not pass secrets to runners assigned to PRs from a forked repository for security reasons, else anybody would have been able to log the secrets by submitting a PR.
That reason is fair, and I don't know how to run tests in a way that mitigates this.
But I think we can just merge this PR (it's pretty clear it works already) and let the tests run on master.

Thanks again for the PR !

Hirevo commented 4 years ago

The tests on master passed indeed ! :tada:

mwilliammyers commented 4 years ago

Ahhh. That’s good to know! Makes sense.

Thanks!