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

Initial commit for appengine deploy action #4998

Merged

Conversation

rachel-fenichel
Copy link
Collaborator

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide

The details

Resolves

#4988

Proposed Changes

  • Adds a gulp action that prepares the demos but doesn't deploy or delete the directory
  • Adds a github workflow with steps to prepare and deploy demos

Behavior Before Change

Deploying had to be done manually.

Behavior After Change

Deploying will be done by clicking a button in github.

Reason for Changes

Automate tasks

Test Coverage

Not yet tested

Documentation

Internal documentation on how to release will need to be updated.

Additional Information

  • I'm following these steps to use the deploy-appengine action. I need to set up a service account and some secrets before I can actually run this.
  • I still need to set the version number based on package.json
  • @cpcallen after your build changes are merged, I'll need to use them and make sure that I'm copying over the correct built files
  • I'm using uploading and downloading artifacts to share information between steps
  • I could pass in a flag to determine whether to switch over traffic to the new version or not (promote: false or promote: true)
  • The prepare step puts files in ../_deploy. I don't know if that's actually accessible from a workflow

Copy link
Contributor

@moniika moniika left a comment

Choose a reason for hiding this comment

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

I understand we're probably going to test this after it's merged in, so I'm going to approve.
I only have a few straightforward comments on the solely on the comments in the workflow file.

.github/workflows/appengine_deploy.yml Show resolved Hide resolved
@@ -0,0 +1,54 @@
# This is a basic workflow to help you get started with Actions
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer this description be updated to describe this specific workflow.

.github/workflows/appengine_deploy.yml Show resolved Hide resolved
.github/workflows/appengine_deploy.yml Show resolved Hide resolved
@moniika
Copy link
Contributor

moniika commented Jul 8, 2021

Re: the promote flag (pros and cons). Our current system where we don't promote when we deploy is nice in that we can test the deploy works fine before migrating traffic. I like that extra protection, but admit that it is a little inconvenient to need to go to patheon and navigate the site to do the migration.

@cpcallen
Copy link
Contributor

cpcallen commented Jul 8, 2021

it is a little inconvenient to need to go to patheon and navigate the site to do the migration.

Since the migrate can be done with the gcloud command line tool, perhaps having the appropriate command presented to the user at a suitable point in time could make life easier?

@rachel-fenichel rachel-fenichel requested a review from a team as a code owner July 9, 2021 02:06
@rachel-fenichel
Copy link
Collaborator Author

Another option is to have separate workflows for deploying and promoting--that way we run them all from github actions, but we still have time to test in before promoting.

Since the migrate can be done with the gcloud command line tool, perhaps having the appropriate command presented to the user at a suitable point in time could make life easier?

One of the goals is to not need to have gcloud installed locally (and set up, and authed) in order to do this.

@rachel-fenichel
Copy link
Collaborator Author

I merged this to develop and tried to test it, but manually running a workflow only works if it's defined in the default branch (master for us) so I'll need to make a PR with these changes against master instead.

The new PR is #5005

@rachel-fenichel rachel-fenichel deleted the deploy_appengine_workflow branch July 9, 2021 02:21
@cpcallen
Copy link
Contributor

cpcallen commented Jul 9, 2021

One of the goals is to not need to have gcloud installed locally (and set up, and authed) in order to do this.

Then my suggestion was terrible and you are right to ignore it!

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.

3 participants