add AWS VPC and EKS reference resources for Karpenter #24
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @lpm0073! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
|
@gabor-boros @jfavellar90 please review once it's ready for review. |
itsjeyd
added
the
needs test run
|
@lpm0073 can you amend the commit messages to follow our conventional commit standard? https://open-edx-proposals.readthedocs.io/en/latest/best-practices/oep-0051-bp-conventional-commits.html#type |
e0d
removed
the
needs test run
itsjeyd
added
the
waiting on author
|
Hey @lpm0073, a friendly reminder to follow up on the comments above. |
|
@bradenmacdonald If I understand correctly, I would need to be a core contributor to review this, which would require a 20h commitment , that I have concerns with from Serenity point of view at the moment. Ref: #25 (comment) |
|
@lpm0073 said on today's call that he is still finding a way to refactor this, to improve the design/code. |
|
@gabor-boros +1 to what @bradenmacdonald said - you don't need to be a core contributor to do the review. Could you schedule the review when it's ready? |
|
@antoviaque @bradenmacdonald Cool! Whenever @lpm0073 says it is ready, I'll schedule a review for it. |
|
The code itself is final. I was unsatisfied with alternative approaches for various reasons. @e0d asked me to modify the commit comments so that these conform to a tCRIL standard. This is still pending. I'll take care of this later today. |
|
Hey @lpm0073, it looks like this PR will need a rebase. Once that's done and the commit linter check passes we'll line it up for engineering review. |
There was a problem hiding this comment.
@lpm0073 I just checked the PR. Very nice work on this!
I mostly have suggestions to bump version numbers, questions about using SSM and some smaller things.
|
Hi @lpm0073, just checking in to see if you're still planning to work on this PR? |
|
Yes, of course, but what's pending?
…On Wed, May 31, 2023, 2:02 AM Tim Krones ***@***.***> wrote:
Hi @lpm0073 <https://github.com/lpm0073>, just checking in to see if
you're still planning to work on this PR?
—
Reply to this email directly, view it on GitHub
<#24 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAI7ZQ2CEFRSDTR46JJ7GFLXI33LFANCNFSM6AAAAAAVRDFSK4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@lpm0073 It looks like there's three things to follow up on:
Once that's done we can line this up for another review pass. |
|
got it, thanks @itsjeyd. I hope to be able to take care of all three later today. |
editing these now. |
… Digital Ocean, based on ./README.md, 'Appendix B: how to create a cluster for testing on DigitalOcean'. Test in AWS us-east-2 data center.
these are done. |
|
Thanks for the updates @lpm0073! It looks like latest master was updated with one or more additional changes that are conflicting/overlapping with the changes from this PR. @gabor-boros To keep things efficient, could we schedule your next review pass for a certain date? That way @lpm0073 would only need to resolve the remaining conflicts one more time (= right before you take another look at the changes). |
itsjeyd
removed
the
waiting on author
|
Thanks, that would be ideal. |
|
@itsjeyd Sure! I'm going to review this today to speed things up! |
There was a problem hiding this comment.
@lpm0073 I just finished reviewing this. I think after a rebase onto latest master branch, this could be merged.
After rebasing, I'm going to test the changes by running terraform, so we can make sure the infra is created properly before merging, but since I'm not expecting any more change requests, I'm giving my approval here.
|
merge conflicts are resolved. Please merge this PR at your earliest convenience. |
|
@lpm0073 Thanks for resolving! I'm going to take the chance to setup a test cluster and merge the MR 😇 |
|
My commit message is improperly formatted 🤦 |
|
I guess "feat: implement Karpenter example" would be a good squash commit message that should pass the test too |
|
yes, perfect :) |
|
this seems kind of unfair. this is a commit that trickled in from the rebase i did last night. 
|
|
Eh, kind of rewording solves the issue, but indeed unfair. I ran out of time today, but will take care of this this week 😇 |
|
@gabor-boros friendly reminder to fix the commit msgs and merge this PR for @lpm0073 please :) |
|
@bradenmacdonald @lpm0073 could we just merge #40 before this one? Once #40 is merged, I will rebase and apply some changes to this PR in order to create a new Harmony chart release. I can fix the commit lint issue as well |
|
@jfavellar90 That's fine with me. Thanks for volunteering to do the rebase! |
itsjeyd
added
the
waiting on author
|
Hey @lpm0073 @jfavellar90, now that #40 has been merged and #41 is up, I'm closing this as a duplicate. Let me know if that's not what you had in mind. |
|
@lpm0073 Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
itsjeyd
added
duplicate
For issue #7: refactor the new infra-example folder into infra-examples, and add Terraform modules for AWS reference resources.
Adds generic AWS Virtual Private Cloud and Elastic Kubernetes Service modules, both of which are preconfigured as necessary to support use of Karpenter for node auto-scaling and pod bin packing.