MarkEdmondson1234 / googleCloudRunner

Easy R scripts on Google Cloud Platform via Cloud Run, Cloud Build and Cloud Scheduler
https://code.markedmondson.me/googleCloudRunner/
Other
81 stars 26 forks source link

Various John commits #122

Closed muschellij2 closed 2 years ago

muschellij2 commented 3 years ago

Easier to maintain scraping code for regions. Probably an API call makes more sense, but this is how I got the regions anyway, so figured I'd make it simpler and updatable. Also, realized region_set didn't work unless same choices were available.

MarkEdmondson1234 commented 3 years ago

Good stuff :) From a quick Google I think this API endpoint would be the one to use for an API call solution https://cloud.google.com/run/docs/reference/rest/v1/projects.locations/list - have put it in an issue to be tackled later https://github.com/MarkEdmondson1234/googleCloudRunner/issues/123

muschellij2 commented 3 years ago

Crud, I added a fix in here for #124 in here as well. PR is polluted, so let me konw how you want to break if you don't want both aspects.

MarkEdmondson1234 commented 3 years ago

No worries, I'll merge them both in. Ideally a test (Even if it only works locally) would also be included for the two issues - they will be run with the test credentials then when I merge into master.

muschellij2 commented 3 years ago

I also just added the argument binary_mode for cr_buildstep_secret as I had a token.rds in there and it doesn't seem to like that unless you get the payload data and base64 decrypt it as per https://cloud.google.com/secret-manager/docs/creating-and-accessing-secrets#a_note_on_resource_consistency

It could be the default without the argument, but I'm unsure if that will screw up any plain text secrets, so figured it'd be better to not make breaking changes.

So I need: 1) Test for different service account (I think I need a name of an account other than the default runner account, unless the test should fail) 2) A test for a binary secret - I think you need to make one with a binary file and then I can use a test to see if it can be read accurately - I think just make a data.frame into an .rds (compressed?) and we can use that as a simple test.

muschellij2 commented 2 years ago

This allows a different service account to be run as per: https://cloud.google.com/build/docs/securing-builds/configure-user-specified-service-accounts

muschellij2 commented 2 years ago

Can we lint the <- issue? I know that's tidystyle but I oppose it pretty hard. My points: = is one key vs. <- which is 2, = is more common for other languages so I can switch easier mentally. I agree <- is more mentally intuitive and is more "mathematically" correct in that = operator isn't an assignment operator in math. Also, it's your package so I'll do what you require, but just wanted to put in my $0.02.

MarkEdmondson1234 commented 2 years ago

I can be persuaded either way on <- vs = but the one thing I think does make code more manageable is it being consistent throughout the code base, so as it's already on <- I would prefer it's all like that.

An auto-linter you mean? That would be cool. Otherwise it will just be me OCDing my way through changing it manually each time I see it ;)

muschellij2 commented 2 years ago

So we can use docker pull us-docker.pkg.dev/google-samples/containers/gke/hello-app:1.0 from https://cloud.google.com/artifact-registry/docs/docker/quickstart. The main thing is that this the form for the artifact registry (vs. container registry).

MarkEdmondson1234 commented 2 years ago

@muschellij2 I would like to merge but can't in the browser as its flagging the .html files as having too complex changes to resolve. I know these are unimportant changes, so if you can put in a commit that will remove the doc changes you made I can merge it in.

Files:

docs/articles/cloudrun.html
docs/news/index.html
docs/pkgdown.yml
docs/reference/RepoSource.html
docs/reference/Source.html
docs/reference/cr_build.html
docs/reference/cr_build_write.html
docs/reference/cr_build_yaml.html
docs/reference/cr_buildstep.html
docs/reference/cr_buildstep_bash.html
docs/reference/cr_buildstep_decrypt.html
docs/reference/cr_buildstep_docker.html
docs/reference/cr_buildstep_git.html
docs/reference/cr_buildstep_mailgun.html
docs/reference/cr_buildstep_nginx_setup.html
docs/reference/cr_buildstep_pkgdown.html
docs/reference/cr_buildtrigger.html
docs/reference/cr_deploy_docker_trigger.html
docs/reference/cr_deploy_packagetests.html
docs/reference/cr_deploy_pkgdown.html
docs/reference/cr_deploy_run_website.html
docs/reference/cr_email_set.html 

Also the linter or something added a lot of whitespace changes and " to ' etc. which made it harder to parse what was important so for next time it would be cool to put linter changes in another pull request.

muschellij2 commented 2 years ago

OK - should be ready to go.

MarkEdmondson1234 commented 2 years ago

Done!