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 GitHub action to deploy the app to develop.simplenote.com #2002

Closed
wants to merge 4 commits into from

Conversation

belcherj
Copy link
Contributor

@belcherj belcherj commented Apr 15, 2020

Reverts #1718

GitHub Actions should be working again. This PR enables auto-deployment to develop.simplenote.com when something gets merged to develop.

I removed the auto-deploy to production until we are comfortable with auto deployments. I would also like to explore auto-deploying any new tags to staging.

@belcherj belcherj self-assigned this Apr 16, 2020
@belcherj belcherj requested a review from a team April 16, 2020 15:41
@belcherj belcherj added this to the 1.16 milestone Apr 16, 2020
@belcherj belcherj marked this pull request as ready for review April 16, 2020 15:41
Copy link
Member

@codebykat codebykat left a comment

Choose a reason for hiding this comment

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

Can we test this in any way before merging it? I mean, it looks fine.

@belcherj
Copy link
Contributor Author

Fixed misspelling. How do I get Grammarly working in vs code and the command? I have become completely reliant on spell-check. :)

@dmsnell
Copy link
Member

dmsnell commented Apr 20, 2020

Besides auto-deploying to develop.simplenote.com what benefit does this change bring us? Could we auto-deploy with our existing CI config?

@belcherj belcherj removed this from the 1.16 milestone Apr 20, 2020
@belcherj
Copy link
Contributor Author

This PR auto-deploys to develop.simplenote.com and adds the capability to auto-deploy to staging and production. We could auto-deploy in CircleCi but the configuration is pretty messy in dealing with GitHub tokens. GH Actions are faster and we don't run into queuing.

@dmsnell
Copy link
Member

dmsnell commented Apr 21, 2020

We could auto-deploy in CircleCi but the configuration is pretty messy in dealing with GitHub tokens.

If they are the ones at settings/keys that makes sense. Do we need to remove Circle configs in this PR so they don't coexist?

@belcherj
Copy link
Contributor Author

If they are the ones at settings/keys that makes sense. Do we need to remove Circle configs in this PR so they don't coexist?

This code only auto-deploys it doesn't do anything that CircleCi does. Im not sure what you are asking here.

@dmsnell
Copy link
Member

dmsnell commented Apr 21, 2020

This code only auto-deploys it doesn't do anything that CircleCi does. Im not sure what you are asking here.

We have bin/deploy.sh already, which is called from npm run deploy so on that part maybe I'm asking more "should we be calling npm run deploy" than about CircleCI.

Also though we have the setup code duplicated in the deploy Dockerfile. Should we be reusing that Dockerfile in both places? is the duplication necessary? the long specific list of Linux dependencies particularly catches my attention as places that are likely to get out of sync and produce subtle differences depending on how we build this.

@belcherj belcherj force-pushed the revert-1718-remove/github-actions branch from 1c70f87 to 431e773 Compare April 27, 2020 16:44
@belcherj
Copy link
Contributor Author

You are right, should definitely use the existing npm run deploy.

The Linux dependencies are different for the two environments. I think we work on deduping as we move the process off of CircleCi.

@belcherj belcherj changed the title Revert "Remove GitHub actions as they can no longer run" Add GitHub action to deploy the app to develop.simplenote.com Apr 27, 2020
runs:
using: 'docker'
image: 'Dockerfile'
entrypoint: './.github/actions/deploy/entrypoint.sh'
Copy link
Member

Choose a reason for hiding this comment

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

the docs indicate that this is here to override the ENTRYPOINT in the Dockerfile. since we're creating the Dockerfile and not overwriting anything I don't understand why we need it, or why we need the entrypoint.sh file at all.

it seems like if it works then things could be kept simpler if we directly embedded the call to npm run deploy

if it's the case that we need this external script in order to pass the BRANCH argument then I think a couple things would help:

  • remove ENTRYPOINT from the Dockerfile
  • add an explanatory comment why we're taking several steps in what seems like it should only take one

the docs don't seem incredibly clear to me. it would be worth verifying if we have access to the ENV vars like BRANCH from inside the Dockerfile without external helpers.

RUN dpkg…

CMD ["sh", "-c", "npm run deploy $BRANCH"]


RUN dpkg --add-architecture i386
RUN apt update
RUN apt -y install python make libxkbfile-dev libxkbfile-dev:i386 libx11-dev libx11-dev:i386 libxss-dev gcc-multilib g++-multilib rpm
Copy link
Member

Choose a reason for hiding this comment

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

fun fact, not the biggest deal here, but each of these lines creates a new "layer" in the Docker image and can carry a reasonable performance hit. since it's an automated build it won't matter too much but it's common to join these into one RUN statement. put differently, we've created these on separate RUN lines for organizational purposes but that creates an unintended side-effect in the Docker image we didn't want.

line-continuation marks and indentation are our friends, and now we can alphabetize the dependencies and make future diffs clearer

RUN dpkg --add-architecture i386 \
    && apt update \
    && apt -y install \
         g++-multilib \
         gcc-multilib \
         libx11-dev libx11-dev:i386 \
         libxkbfile-dev libxkbfile-dev:i386 \
         libxss-dev \
         make \
         python \
         rpm

@dmsnell
Copy link
Member

dmsnell commented Apr 28, 2020

Asking out of ignorance: will this impact #2042? I notice that we have to create a Docker environment and add the i386 architecture, presumably to install build deps for it? Would we need to, would we be able to, still cross-compile for ARM?

@belcherj
Copy link
Contributor Author

Closing as much has changed and I think we need to modify the build in ways that would make this PR obsolete. We shouldn't build the desktop app to ship the web app.

@belcherj belcherj closed this Mar 10, 2021
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.

None yet

3 participants