-
Notifications
You must be signed in to change notification settings - Fork 136
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
Introduce Google Cloud Run deployment example #292
Conversation
Signed-off-by: Adam Pike <adam.pike@1password.com>
Signed-off-by: Adam Pike <adam.pike@1password.com>
- must be quoted Signed-off-by: Adam Pike <adam.pike@1password.com>
Signed-off-by: Adam Pike <adam.pike@1password.com>
- don't scale to 0 - faster boot time Signed-off-by: Adam Pike <adam.pike@1password.com>
- must total 1 vCPU Signed-off-by: Adam Pike <adam.pike@1password.com>
Signed-off-by: Adam Pike <adam.pike@1password.com>
Signed-off-by: Adam Pike <adam.pike@1password.com>
- shorter name - add subfolder and YAML spec for connecting to Google Workspace Signed-off-by: Adam Pike <adam.pike@1password.com>
- most ToC entries are missing - GW section is missing some steps - needs appendices to cover updates, scaling, cred rotation, etc. Signed-off-by: Adam Pike <adam.pike@1password.com>
abc8076
to
a4b4c19
Compare
Signed-off-by: Adam Pike <adam.pike@1password.com>
- fix alert rendering Signed-off-by: Adam Pike <adam.pike@1password.com>
- remove indents for quote blocks used for alerts to enable rich rendering Signed-off-by: Adam Pike <adam.pike@1password.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the improvements. There a few small typos or sections we overlooked.
Thanks for the look! I've got some local work that is almost ready to commit that resolves a bit of this already. 🙂 |
- completed steps to enable Workspace - incorporated review suggestions - typos/corrections Signed-off-by: Adam Pike <adam.pike@1password.com> Co-Authored-By: Bryan Black <bryan.black@agilebits.com>
Signed-off-by: Adam Pike <adam.pike@1password.com>
- add README.md to google-workspace subdir - remove GW instructions from main README - add callout to link out to GW instructions in new README Co-Authored-By: Bryan Black <bryan.black@agilebits.com>
- shorter, better tooltip for GW steps - migrated info from tooltip to GW README Signed-off-by: Adam Pike <adam.pike@1password.com>
- functions corretly either way - aligned with indent structure for lists elsewhere in this file and repo Signed-off-by: Adam Pike <adam.pike@1password.com>
- fix broken relative links introduced in last commit - punctuation Signed-off-by: Adam Pike <adam.pike@1password.com>
- ToC - prereq steps Signed-off-by: Adam Pike <adam.pike@1password.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not run through this yet from a validation standpoint with the updated GW steps. I will try and take it for a run soon.
A few small suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review! I'll address some of these in a future commit. Some comments with suggestions on how to do some of those suggestions. ;-)
- added updated instructions - style (case) Co-authored-by: Bryan Black <bryan.black@agilebits.com>
Signed-off-by: Adam Pike <adam.pike@1password.com>
Co-Authored-By: Bryan Black <bryan.black@agilebits.com>
c6f25e0
to
39694ef
Compare
Signed-off-by: Adam Pike <adam.pike@1password.com>
Signed-off-by: Adam Pike <adam.pike@1password.com>
Signed-off-by: Adam Pike <adam.pike@1password.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good. We just need to apply the URL todo changes to ensure that the paths match up as part of the merge
All links should be pointing the their new home on |
- fixed some broken links - corrected typos - slightly shorter code block for secret access command - clearer update steps - DRY-er post-update testing - better callouts Signed-off-by: Adam Pike <adam.pike@1password.com>
Signed-off-by: Adam Pike <adam.pike@1password.com>
Signed-off-by: Adam Pike <adam.pike@1password.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small suggestion about how we handle calling the project name in the long command.
```sh | ||
gcloud secrets add-iam-policy-binding scimsession --member=serviceAccount:$( | ||
gcloud iam service-accounts list --filter="$( | ||
gcloud projects describe op-scim-bridge --format="value(projectNumber)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to add a tip here, stating that if they used a custom project name to edit this command.
As an alternative to a tip, we could set a opProject
variable as part of the command used in step 1.2 and the tip in step 1.2 for customers who already created a project. This way we could simply use that within this long command to remove any confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point, and I think we can likely improve the flow a bit in that earlier step and perhaps replace this entire line in this block with that variable.
Let's merge this as is and then I'll note it in a new issue.
This PR is to establish a new Cloud Run deployment example for SCIM Bridge, including:
For review:
/beta/google-cloud-run/README.md
/beta/google-cloud-run/google-workspace/README.md