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

Various John commits #122

Merged
merged 56 commits into from
Nov 26, 2021
Merged

Conversation

muschellij2
Copy link
Contributor

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
Copy link
Owner

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 #123

@muschellij2
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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.

…ecursive nesting if you run deploy_docker from the sam edirectory as the dockerfile
@muschellij2
Copy link
Contributor Author

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

NAMESPACE Outdated Show resolved Hide resolved
R/docker.R Outdated
@@ -85,6 +85,9 @@ cr_deploy_docker_trigger <- function(repo,
#'
#' To deploy builds on git triggers and sources such as GitHub, see the examples of \link{cr_buildstep_docker} or the use cases on the website
#'
#' @note `construct_cr_deploy_docker` is a helper function to construct the arguments

Choose a reason for hiding this comment

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

Could I see an example of its usage?

R/docker.R Outdated Show resolved Hide resolved
@muschellij2
Copy link
Contributor Author

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
Copy link
Owner

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 ;)

R/utilities.R Outdated Show resolved Hide resolved
@muschellij2
Copy link
Contributor Author

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
Copy link
Owner

MarkEdmondson1234 commented Nov 25, 2021

@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
Copy link
Contributor Author

OK - should be ready to go.

@MarkEdmondson1234 MarkEdmondson1234 merged commit ad0742c into MarkEdmondson1234:master Nov 26, 2021
@MarkEdmondson1234
Copy link
Owner

Done!

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