-
Notifications
You must be signed in to change notification settings - Fork 116
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
Fix amplify push twice #3493
Conversation
the logic as written will run the init command twice if it returns a non-zero exit code for any reason. see https://github.com/koalaman/shellcheck/wiki/SC2015
@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. |
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 |
@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 I've also cleaned up some sources of issues elsewhere. I'm a little unsure what the resolution of the "envCache only writes lowcase keys 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.
|
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. |
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.
|
So after much pain I have come to the conclusion that Amplify is applying a string replacement for the command If it is the first build on a detected branch, the string is replaced with Replacement A:
on subsequent builds it'll replace it with Replacement B:
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. |
The script appears to be informed by if If 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. |
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
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.