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

Fix amplify push twice #3493

Closed
wants to merge 8 commits into from

Conversation

joekiller
Copy link

Description of changes

This fixes the bash logic which would result in the init running more than once. I've seen plenty of confused people due to this subtle bash problem. See SC2015 for the logical fallacy that the amplify init commands fall into.

I also updated amplifyPush to reflect the latest version being used and fixed a few other potential sources of error.

Issue #, if available

likely fixes stuff like #2957, #2291, and #924

Checklist

  • PR description included

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@joekiller joekiller requested a review from a team as a code owner May 24, 2023 20:04
@joekiller
Copy link
Author

@josefaidt while working on a stack similar to yours in aws-amplify/amplify-cli#12644 I identified that 167a4df fixes the ancient weird problem of amplifyPush pushing twice sometimes.

@calavera
Copy link
Contributor

As stated in the file, we don't use this script anymore. We should probably just remove it. Adding any code in there will only confuse people more than they already are when they try to use amplifyPush.

@joekiller
Copy link
Author

@calavera that's a fair point and I do understand that it is deprecated. Along the lines of "it's no longer used" I think the reason I updated this script is pertinent. I wanted to see what new CI build would be suggested to me as I knew the amplifyPush was deprecated and I was curious what would be given to me when I added a new amplify project via the "add github deployment" feature of the UI without a build.

To my surprise it suggested the following build was given that included the script. I then cat $(which amplifyPush) and saw the script is newer than anything else. Because this double push was happening I rant the linter on the script and discovered the bug.

I've also cleaned up some sources of issues elsewhere.

I'm a little unsure what the resolution of the "envCache only writes lowcase keysstackinfo -> stackInfo" issue was fixed as it appears y'all are using the stackInfo key on the ENVIRONMENT_CACHE_READ. Also the "amplify generated backend" names persist across the environment builds in an unclear way. I'm assuming y'all have like a envCache key you are using but I haven't gotten to the point of looking.

Here is the current build that is given by the Amplify console when I added my project without a build. It was a bare javascript project.

I'd expect if the script was fully deprecated then the "correct" build would be given.

version: 1
backend:
  phases:
    build:
      commands:
        - '# Execute Amplify CLI with the helper script'
        - amplifyPush --simple
frontend:
  phases:
    build:
      commands: []
  artifacts:
    # IMPORTANT - Please verify your build output directory
    baseDirectory: /
    files:
      - '**/*'
  cache:
    paths: []

@joekiller
Copy link
Author

It feels like y'all are hoisting in a custom "amplifyPush" command in your build shell or something. It's really hard to decipher the differences in behaviors other than, the amplifyPush on the file system isn't used. Y'all inject a custom amplifyPush into the shell running the build or something via https://github.com/vercel/pkg

Is the new push just in the amplify-cli somewhere? It would be helpful to understand the mechanism to reconcile how to handle feature branch builds with random names being given and ourselves doing an init with force push or whatever.

Currently I cannot remove the --forcePush from the build using a sed -i 's/--forcePush//g' /usr/bin/amplifyPush.

I'd like to remove it and keep the rest of the functionality.

@joekiller
Copy link
Author

I was wrong about the default script, I was given the following and then followed in the docs Deploying the backend with the front end.

amplifyPush effectively smooths over the various edge cases of feature branch deployments

With out it there are questions like, "how does the environment name persist and get presented to the build over time?" or "How is the CLI supposed to resolve those questions?" There are many conventions that amplify's build seems to be employing but are not shared. Previously they were shared in the amplifyPush.sh but now we are being told "it's not used" however it is clearly being bundled into the build client (not the file system) and still updated to take care of edge cases.

version: 1
frontend:
  phases:
    # IMPORTANT - Please verify your build commands
    build:
      commands: []
  artifacts:
    # IMPORTANT - Please verify your build output directory
    baseDirectory: /
    files:
      - '**/*'
  cache:
    paths: []

@joekiller
Copy link
Author

joekiller commented May 25, 2023

So after much pain I have come to the conclusion that Amplify is applying a string replacement for the command amplifyPush --simple. It will replace that string with one of two strings depending on if the build is the first build or if it is a subsequent build. The script seems to go to great lengths to obscure this fact.Y'all should update the docs and make it transparent that script is a macro now. The following are the mystery replacements that I cannot sed.

If it is the first build on a detected branch, the string is replaced with Replacement A:

amplify init --amplify "{\"envName\":\"randomEnvName\",\"appId\":\"abcXYZ\"}" --providers "{\"awscloudformation\":{\"configLevel\":\"project\",\"useProfile\":true,\"profileName\":\"default\"}}" --codegen "{\"generateCode\":false,\"generateDocs\":false}" --yes  --forcePush;

on subsequent builds it'll replace it with Replacement B:

amplify pull --amplify "{\"envName\":\"randomEnvName\",\"appId\":\"abcXYZ\"}" --providers "{\"awscloudformation\":{\"configLevel\":\"project\",\"useProfile\":true,\"profileName\":\"default\"}}" --no-override --no-codegen --yes  && amplify init --amplify "{\"envName\":\"randomEnvName\",\"appId\":\"abcXYZ\"}" --providers "{\"awscloudformation\":{\"configLevel\":\"project\",\"useProfile\":true,\"profileName\":\"default\"}}" --codegen "{\"generateCode\":false,\"generateDocs\":false}" --yes  --forcePush;

If you delete the backend that the environment was hooked up to then the build will apply Replacement A again.

If you clone a backend and attach the build to the existing backend, it will use Replacement B.

If you delete the backend on CFN but still have the environment in Amplify, it will use Replace B and fail. (Kinda expected).

It's unclear if Amplify will replace and use other plugin headless commands if you have auth enabled from a third party. I will not explore any further.

I guess I will close this with the general statement that I believe it should be documented that amplifyPush --simple is being replaced "smartly" and outline what is best practice to eliminate this misdirection of command replacement.

@joekiller joekiller closed this May 25, 2023
@joekiller
Copy link
Author

The script appears to be informed by if REMOTE_BACKEND_EXISTS=true or REMOTE_BACKEND_EXISTS=false and if AWS_BACKEND_ENVIRONMENT_ARN is defined or not. If AWS_BACKEND_ENVIRONMENT_ARN is defined. When the backend exists, it pulls and then does a force init on that env, if it doesn't it it force inits it.

If AWS_BACKEND_ENVIRONMENT_ARN is not present it appears to use the USER_BRANCH environment value.

If AWS is making a backend and the branch name doesn't fit convention, they will set the USER_BRANCH for the first build.

If you attached a build to a new environment USER_BRANCH will be set to the name of the new target.

If AWS is making a backend and the branch name fits convention, they will set AWS_BRANCH.

If you attached a build that has a branch that fits convention to an existing environment USER_BRANCH will be set along with AWS_BRANCH, thus making USER_BRANCH take prescedence.

If you attached a build that has a branch that fits convention to the environment matching it's name it'll have USER_BRANCH and AWS_BRANCH set to the same thing.

If you detach your build that has a branch that fits convention and already have a backend that matches the branch name, then only AWS_BRANCH will be set.

@joekiller joekiller deleted the fix-amplify-push-twice branch May 25, 2023 23:30
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.

2 participants