PR-1123 Add Support for Graviton 2 / ARM Architecture (conflict fix)

[gaborschulz]

Description

PR-1123 Add Support for Graviton 2 / ARM Architecture has merge conflicts. This PR intends to fix those conflicts.

GitHub Issues

Resolves: #1051

[coveralls]

coverage: 74.688% (+0.08%) from 74.605% when pulling e118011 on gaborschulz:master into 26d6182 on zappa:master.

[monkut]

Can you add a testcase to cover defining/not defining the architecture value and confirming the resulting settings?

[gaborschulz]

Can you add a testcase to cover defining/not defining the architecture value and confirming the resulting settings?

Sure, in which test file should I put it?

[monkut]

settings related tests are located in tests.py

[monkut]

looks like linter is failing, needs make black, make isort.

[gaborschulz]

looks like linter is failing, needs make black, make isort.

Fixed. I've also added pre-commit hook to prevent this from happening again in the future.

[JRemitz]

Any updates on when this will be merged or if there are outstanding changes needed? Would be great to be able to specify the architecture.

[javulticat]

So, unfortunately, if I understand what you will be attempting to do with merging this PR into master, hoping it will fix the errors PR #1123 is encountering and allowing it to then be merged into master as well, I'm compelled to tell you that doing something like that is typically a practice that should be avoided in the strongest possible terms. Opening a PR (#1209) to merge into master in the hope that it will fix problems that are occurring in another PR (#1123) is something we should never be doing outside of the most extraordinary circumstances (and I can't think of any that would qualify).

Please correct me if I have misunderstood, but it looks like that's basically what is about to go down here: two PRs that are related to each other are both about to be merged into master, hoping that everything will just kinda "work out" once both PRs are merged. But, I'm telling you, it is far more likely that not only are all of #1123's problems not solved, but it's also more likely that master will start experiencing various issues of its own (test failures, preventing merging any other PRs going forward, and a handful of other things that will require a lot more cleanup work to get things back in working order than the amount of work it would take to safely merge both of this PRs into master without any of that risk.

The TL;DR of how you'd do that is:

1. You'd change the target branch for this PR (PR-1123 Add Support for Graviton 2 / ARM Architecture (conflict fix) #1209) from master to instead being the feature branch that was used to open PR Add Support for Graviton 2 / ARM Architecture #1123.
2. Tests will then run against the code from PR-1123 Add Support for Graviton 2 / ARM Architecture (conflict fix) #1209 combined with the code from Add Support for Graviton 2 / ARM Architecture #1123. Assuming those tests pass, merge this PR (merging this PR's feature branch into Add Support for Graviton 2 / ARM Architecture #1123's feature branch - remember: NOT INTO master)
3. Now you should only have a single relevant PR remaining (Add Support for Graviton 2 / ARM Architecture #1123), but it now contains the code found in this PR (PR-1123 Add Support for Graviton 2 / ARM Architecture (conflict fix) #1209) as well. The addition of this new code will trigger our CI to run new tests for PR Add Support for Graviton 2 / ARM Architecture #1123 against master.
4. Assuming the tests for Add Support for Graviton 2 / ARM Architecture #1123 running against master now pass thanks to the addition of your code from PR-1123 Add Support for Graviton 2 / ARM Architecture (conflict fix) #1209, it should now be safe to merge Add Support for Graviton 2 / ARM Architecture #1123 into master (assuming it doesn't have any unrelated issues still outstanding).
5. As you can see, we merged PR-1123 Add Support for Graviton 2 / ARM Architecture (conflict fix) #1209's feature branch into Add Support for Graviton 2 / ARM Architecture #1123's feature branch, and only after confirming everything looked good (all tests pass, etc.), then we merged Add Support for Graviton 2 / ARM Architecture #1123 into master. The key part here is that we only merged a single PR into master, not both!. But, despite that fact, we still managed to merge the code from both PRs into master without much trouble - the best of both worlds.

Significantly Longer and Much More In-Depth Explanation of the Same Basic Principles Can Be Found Below - You've Been Warned! 😈

The reason that merging multiple closely-related PRs into master can end up very "bad" is that doing so can nearly entirely eliminate any of the benefits we normally get from requiring that our CI tests pass before a branch can be merged. Since these two PRs and their relevant feature branches are inexorably linked both to each other and to master (e.g., #1209 fixes problems with #1123, #1123 fixes/adds something to master), if they are both merged directly into master, then our test cases won't get run by the CI against the combined result of #1209 + #1123 + master code until after all of the code has already been merged and combined in master IRL.

And that's something we'd like to try to avoid if at all possible, because, while this PR (#1209) appears to be passing tests against the code in master currently, that gives us no indication on whether or not those tests running against master will continue to pass once PR #1123 is thrown into the mix, whch has its tests against master currently failing. Without a way to combine the code from both #1209 and #1123 and test it against master beforehand, then essentially the first time that the full combination of both PRs will be tested against master will be in the CI pipeline that runs after code is merged into master. It's essentially the CI version of testing in production, which is why it should be avoided.

Doing this actually carries a significant enough risk of causing a "disaster" according to GitLab (GitHub's main competitor) - as the lack of proper testing can easily obscure the fact that the code in your master branch will fail tests, behave unexpectedly, or not even run at all once both PRs have been merged into it. And then if you have your CI/CD configured to automatically deploy new commits made to master into your production environment, then you have, well, a pretty legitimate "disaster" I suppose.

That is actually one of the big reasons why GitLab has dedicated a large number of resources to this problem and has been constantly adding various relevant new features in recent years that attempt to prevent users from possibly being able to make this mistake and risk a "disaster". And a lot of really neat and useful features that can be used for a lot of other interesting purposes have actually come out of those efforts. But, unfortunately, GitHub doesn't seem to have any of the same set of features in that area, AFAIK, so trying to avoid these situations manually seems to be the best we can do for now on GitHub.

So, we've talked doom and gloom, but what should be done, ideally, in cases like these? Well gee, I'm glad you asked. The best practice here would be to open a PR which merges this PR's branch (gaborschulz:master) directly into the branch being used in PR #1123 (NinitoAS:ARM64_Support), rather than attempting to merge this PR (#1209) directly into themaster branch at all. Opening a PR that targets the feature branch of the PR (#1123) that this PR (#1209) is supposed to fix will cause helpful tests to be generated and run that will be able to prove that the code in #1209 definitely works well with the code in #1123 (because the test cases that will be run if the PR targets #1123's feature branch will be run against what #1123 would look like if #1209 was merged into it - which is exactly what we'd need to see if we wanted to know if the changes in #1209 would cause any problems in #1123).

And, because we're merging #1209 into #1123's feature branch instead of the master branch now, this will no longer carry the risk of possibly breaking something in the master branch or breaking/causing conflicts with any other PRs being merged into master simultaneously, because this PR (#1209) should now be getting merged directly into #1123's feature branch without touching or having anything to do with master (yet).

Once we do merge this PR (#1209) directly into #1123's feature branch (leaving us with just one remaining PR: #1123 - although, it now contains the code that was in #1209 as well),.then we just need to wrap the process up and merge the original PR (#1123) into master). Assuming the PR to merge #1209 into #1123 completed and was merged successfully, more useful tests will automatically generate and run on the #1123 PR (because the code changes from #1209 will be detected by Github since #1209 will have been merged into it), and those tests will be able to prove that, thanks to the inclusion of #1209's changes, whether #1123 is now able to pass its test suite that runs against master, and, if it passes, then that would indicate it now should be safe to merge #1123 (and, by extension, the code from #1209 that now is a part of #1123) into master. In other words, it shows that #1209, #1123, and master all play nice together, which is what we needed to show in order to have confidence in our merge procedure here.

To be explicit here, on the path you're currently heading down, there will only be largely useless tests that run that won't tell us anything about whether both of these PRs will play nice with both each other and master at the same time, and the need to carefully coordinate merging both PRs into master one after another while somehow ensuring across a large OSS project that no other PR will begin to get merged at the same time (which may very well end up causing a bunch of merge conflicts again, and we're back where we started) might be required. 😱

Alternatively, by merging the code from this PR (#1209) directly into the feature branch for the PR (#1123) that this code is meant to help fix/resolve conflicts for, you'll end up getting multiple rounds of helpful and informative tests that will give you high confidence that both this PR successfully fixes #1123 and that both of the PRs will work well together with master. You'll also definitely have no need to worry about timing these two PRs to merge into master right after the other in a certain order while hoping that no other PR comes along to be merged while you are doing that.

Hopefully I've explained that sufficiently well enough that we are all in agreement that this PR's target branch should not be master any longer, but should instead be changed to target merging into the feature branch of PR #1123 🙂 But, please don't hesitate to let me know if I can try to clarify anything further! (Or if I rambled far too much in this reply for anyone to really follow what the hell I was saying, but, hey, that's pretty much a given! 🙃)

[github-actions]

Hi there! Unfortunately, this PR has not seen any activity for at least 90 days. If the PR is still relevant to the latest version of Zappa, please comment within the next 10 days if you wish to keep it open. Otherwise, it will be automatically closed.

[github-actions]

Hi there! Unfortunately, this PR was automatically closed as it had not seen any activity in at least 100 days. If the PR is still relevant to the latest version of Zappa, please open a new PR.

[simsong]

@javulticat - I appreciate the answer above. However, this is functionality that I need now, and the authors seem to have abandoned it. What do you recommend I do?

[javulticat]

I'll be implementing ARM support as part of Zappa's next release (0.60.0) cycle. To get this functionality ASAP, I recommend keeping an eye on #1051. Once that Issue is closed by a PR, you could install Zappa directly from master (or the specific commit that adds ARM support) in the interim before version 0.60.0 is officially released on PyPi (see here for instructions on how to install Zappa directly from GitHub).