- 
                Notifications
    You must be signed in to change notification settings 
- Fork 121
Add helper script for creating deployment connectivity resources #1306
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
base: main
Are you sure you want to change the base?
Conversation
| ✅ Deploy Preview will be available once build job completes!
 | 
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'm personally good with this, but will leave it to @JTorreG to approve and merge
        
          
                content/nginxaas-google/getting-started/create-deployment/deploy-console.md
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                content/nginxaas-google/getting-started/create-deployment/deploy-console.md
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | {{< details summary="Show helper script" >}} | ||
|  | ||
| ```bash | ||
| #!/bin/bash | 
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 think we should always apply set -eo pipefail in any bash scripts, otherwise poor error handling can lead to unexpected results.
I'd also suggest -u to ensure any expected env vars are set (set -euo pipefail)
        
          
                content/nginxaas-google/getting-started/create-deployment/deploy-console.md
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | # Set auto-generated proxy subnet name and VIP name | ||
| PROXY_SUBNET="psc-proxy-subnet" | ||
| VIPNAME="psc-vip" | 
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.
nit: move these up to the top with the other default env vars. even though there isn't a command line flag to modify them right now, a user might want to just change what is hardcoded and it's easier to find at the top
        
          
                content/nginxaas-google/getting-started/create-deployment/deploy-console.md
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | - For **Port number**, enter the same port as your NEG's Producer port, for example, port `80`. | ||
|  | ||
|  | ||
| Each listening port configured on NGINX requires its own network endpoint group with a matching port. You can use the following helper script to automate these steps: | 
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.
| Each listening port configured on NGINX requires its own network endpoint group with a matching port. You can use the following helper script to automate these steps: | |
| Each listening port configured on NGINXaaS requires its own network endpoint group with a matching port. You can use the following helper script to automate these steps: | 
it's a nit but let's talk in terms of the service rather than what it has underneath.
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.
separately, let's be specific about the type of NEG, PSC NEG to avoid confusion as there are various types of NEGs in GCP.
| --purpose=REGIONAL_MANAGED_PROXY \ | ||
| --role=ACTIVE | ||
| echo "Created proxy-only subnet: $PROXY_SUBNET" | ||
| else | 
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.
you can safely drop this and just state that you are using the subnet whether it exists or was just created.
or we fix up the rest of the script with this if-else logic to stay consistent.
| exit 1 | ||
| fi | ||
| # Create proxy-only subnet (skip if exists) | 
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.
this statement can be echoed out instead of being a comment. The comment applies throughout the script where you do so.
| gcloud compute addresses create $VIPNAME --region=$REGION --project=$PROJECT --network-tier=PREMIUM | ||
| fi | ||
| VIP=$(gcloud compute addresses describe $VIPNAME --region=$REGION --project=$PROJECT --format='get(address)') | ||
| echo "Using VIP address: $VIP" | 
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.
is this a VIP or a static public IP that you are reserving on Google?
| # Create regional VIP address (skip if exists) | ||
| echo "Creating regional VIP address..." | ||
| if ! gcloud compute addresses describe $VIPNAME --region=$REGION --project=$PROJECT >/dev/null 2>&1; then | ||
| gcloud compute addresses create $VIPNAME --region=$REGION --project=$PROJECT --network-tier=PREMIUM | 
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.
--network-tier=PREMIUM I don't think it needs to be premium?
Proposed changes
Checklist
Before sharing this pull request, I completed the following checklist:
Footnotes
Potentially sensitive information includes personally identify information (PII), authentication credentials, and live URLs. Refer to the style guide for guidance about placeholder content. ↩