-
Notifications
You must be signed in to change notification settings - Fork 186
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 from source sample #216
base: main
Are you sure you want to change the base?
Conversation
/fixes #116 |
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.
Some wording and file changes. From a functional standpoint, looks good - deploys and runs as advertised when configured as described in README.
@@ -0,0 +1,32 @@ | |||
PACKAGE = github.com/Azure-Samples/azure-sdk-for-go-samples/apps/basic_web_app |
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 issue with using a makefile is that it excludes users who are in PowerShell environments. It's up to you whether or not we want to explicitly provide build support in the samples for pwsh users, or if we want to only focus on Linux/WSL for automated builds, but it's something to consider. This also affects setup.sh
.
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.
Also did a quick check to see if make
is called anywhere - it's not. This file is used for container/binary deploy only, not source, and it can be removed.
@@ -0,0 +1,15 @@ | |||
function ensure_group () { |
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 function definition should just be rolled into the setup.sh
script, unless it's intended to be used elsewhere - in that case it should go into some kind of generic utils
package. If it is it still needs to be deployable.
**NOTE**: GitHub sync 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 |
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.
Don't suggest doing this, it's a security risk since it publishes an auth token to a public resource.
|
||
AZURE_BASE_NAME=go-webapp-tester | ||
AZURE_DEFAULT_LOCATION=westus2 | ||
|
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.
Remove l7-8 from the template.
specified repo. Don't forget to `git push` your code there too! | ||
|
||
**NOTE**: GitHub sync requires a [GitHub personal access | ||
token](https://github.com/settings/tokens); you need to get one from the linked |
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.
Indicate the level of permissions required for the token in order for the deployment to operate. Assuming it's repo
?
exit $err_nogithubtoken | ||
fi | ||
|
||
#### |
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 get some general output here, but maybe run the commands with --verbose
so that more progress information from the CLI is displayed?
--is-linux \ | ||
--output tsv --query id) | ||
fi | ||
echo "ensured plan $plan_id" |
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.
Weird verb choice here. Change to Created
or Set
maybe?
--output tsv --query id) | ||
|
||
if [[ -z $plan_id ]]; then | ||
plan_id=$(az appservice plan create \ |
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.
If I remember right, this creates a plan with a basic S1
tier (FREE
is not allowed for Linux containers for some reason) so users should be advised to clean up when finished. Maybe a cleanup script should be provided as well?
--location $location \ | ||
--query 'id' --output tsv) | ||
fi | ||
echo $group_id |
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.
Provide more context. It might also be useful to echo the group name rather than the group ID.
--output tsv --query 'id') | ||
echo "ensured web app: $webapp_id" | ||
|
||
## ensure operation |
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.
Add an echo
here to indicate that the test is being run. Maybe:
echo "Testing deployment..."
Similar to #215 but without containers, this sample includes a basic Go web app and a script to set up infrastructure and configuration in App Service to continuously build and deploy this app. See README.md for full details. It only works if copied to the root of a user's own directory, as in https://github.com/joshgav/go-sample-source.