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

add webapp & container registry/builder sample #215

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joshgav
Copy link
Contributor

@joshgav joshgav commented Aug 14, 2018

This includes a basic Go web app and a script to set up Azure infrastructure in App Service and Container Registry to continuously build and deploy this app. See README.md for full details.

If ./setup.sh is run in this repo (i.e. a clone) it will ensure/create Azure resources then push/build the image once.

If ./setup.sh is run from a repo root and IMAGE_NAME is set to the repo's name, continuous build and deploy is set up with an ACR build task. See https://github.com/joshgav/go-sample for an example of creating your own web app from this sample.

@joshgav joshgav added the review label Aug 14, 2018
@joshgav joshgav requested a review from marstr August 14, 2018 19:26
@joshgav
Copy link
Contributor Author

joshgav commented Aug 14, 2018

PTAL @sptramer @asw101

@joshgav
Copy link
Contributor Author

joshgav commented Aug 14, 2018

fixes #40

Copy link
Contributor

@sptramer sptramer left a comment

Choose a reason for hiding this comment

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

Some cleanup needed.

@@ -0,0 +1,7 @@
[prune]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can safely be removed. Project has no external dependencies that need to be locked.

@@ -0,0 +1,20 @@
# This file is autogenerated, do not edit; changes may be undone by the next 'dep ensure'.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove along with Gopkg.toml

AZURE_BASE_NAME=go-webapp-tester
AZURE_DEFAULT_LOCATION=westus2

# required for continous container build setup
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove GH_TOKEN from environment

**NOTE**: Container Registry Build requires a [GitHub personal access
token](https://github.com/settings/tokens); you need to get one from the linked
page and set it in a local environment variable `GH_TOKEN`, e.g. `export
GH_TOKEN=mylongtokenstring`. You can also add it to your local `.env` file for
Copy link
Contributor

Choose a reason for hiding this comment

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

Security risk; don't suggest adding to env.

@@ -0,0 +1,32 @@
PACKAGE = github.com/Azure-Samples/azure-sdk-for-go-samples/apps/basic_web_app
Copy link
Contributor

Choose a reason for hiding this comment

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

make is never used. Can be deleted.

@@ -0,0 +1,15 @@
function ensure_group () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be merged into setup.sh or some common utility location. Shared between here and #216

@@ -0,0 +1,33 @@
package main
Copy link
Contributor

Choose a reason for hiding this comment

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

Shared between here and #216. It may not be a bad idea to merge these PRs together, and have setup.sh or a PowerShell script take an argument; deploy-X where X is some type of app deployment. They all use the same environment variables, and individual deploys could be broken out into a scripts directory and called from the setup.sh (or sourced into it.)

@@ -0,0 +1,193 @@
#!/usr/bin/env bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding a PowerShell script so that Windows users without WSL are supported. It's unclear if this is a path that we want to push with the Go story but it seems like it's important to keep those users in mind, especially since there are a significant number of Azure users in the Windows ecosystem.


It uses the following environment variables to choose names:

* IMAGE\_NAME: Name of container image (aka "repo").
Copy link
Contributor

Choose a reason for hiding this comment

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

IMAGE_NAME is not a natural choice here; maybe REPO_NAME, and spell out that the repo name is used for the image name?

ensure_group $group_name

## ensure registry
registry_id=$(az acr show \
Copy link
Contributor

Choose a reason for hiding this comment

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

At some point here during my initial test, the script hung, but I can't see what would have caused it. There's the possibility that it was caused by Docker not running locally, but would need to investigate further. I can say for sure that the resource group was created, but had no resources allocated to it.

@sptramer
Copy link
Contributor

Should mention that most comments regarding wording, etc. from #216 also apply here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants