openedx / openedx-k8s-harmony

A Prototype Helm Chart for deploying multiple Open edX instances (via Tutor) onto a cluster.
GNU Affero General Public License v3.0
10 stars 14 forks source link

add AWS VPC and EKS reference resources for Karpenter #24

Closed lpm0073 closed 1 year ago

lpm0073 commented 1 year ago

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.

openedx-webhooks commented 1 year ago

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.

bradenmacdonald commented 1 year ago

@gabor-boros @jfavellar90 please review once it's ready for review.

e0d commented 1 year ago

@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

itsjeyd commented 1 year ago

@lpm0073 Thank you for your contribution!

In addition to changing the commit messages as @e0d mentioned above, it would be great if you could bring your branch up-to-date with the main branch.

Once that's done this PR will be ready for engineering review.

itsjeyd commented 1 year ago

Hey @lpm0073, a friendly reminder to follow up on the comments above.

gabor-boros commented 1 year ago

@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: https://github.com/openedx/openedx-k8s-harmony/issues/25#issuecomment-1477716458

bradenmacdonald commented 1 year ago

@lpm0073 said on today's call that he is still finding a way to refactor this, to improve the design/code.

antoviaque commented 1 year ago

@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?

gabor-boros commented 1 year ago

@antoviaque @bradenmacdonald Cool! Whenever @lpm0073 says it is ready, I'll schedule a review for it.

lpm0073 commented 1 year ago

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.

itsjeyd commented 1 year ago

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.

itsjeyd commented 1 year ago

Hi @lpm0073, just checking in to see if you're still planning to work on this PR?

lpm0073 commented 1 year ago

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 https://github.com/openedx/openedx-k8s-harmony/pull/24#issuecomment-1569691339, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAI7ZQ2CEFRSDTR46JJ7GFLXI33LFANCNFSM6AAAAAAVRDFSK4 . You are receiving this because you were mentioned.Message ID: @.***>

itsjeyd commented 1 year ago

@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.

lpm0073 commented 1 year ago

got it, thanks @itsjeyd. I hope to be able to take care of all three later today.

lpm0073 commented 1 year ago

@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

editing these now.

lpm0073 commented 1 year ago

@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.

these are done.

itsjeyd commented 1 year ago

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).

lpm0073 commented 1 year ago

Thanks, that would be ideal.

gabor-boros commented 1 year ago

@itsjeyd Sure! I'm going to review this today to speed things up!

lpm0073 commented 1 year ago

merge conflicts are resolved. Please merge this PR at your earliest convenience.

gabor-boros commented 1 year ago

@lpm0073 Thanks for resolving! I'm going to take the chance to setup a test cluster and merge the MR 😇

lpm0073 commented 1 year ago

My commit message is improperly formatted 🤦

gabor-boros commented 1 year ago

I guess "feat: implement Karpenter example" would be a good squash commit message that should pass the test too

lpm0073 commented 1 year ago

yes, perfect :)

lpm0073 commented 1 year ago

this seems kind of unfair. this is a commit that trickled in from the rebase i did last night.

Screenshot 2023-06-19 at 10 09 11
gabor-boros commented 1 year ago

Eh, kind of rewording solves the issue, but indeed unfair. I ran out of time today, but will take care of this this week 😇

bradenmacdonald commented 1 year ago

@gabor-boros friendly reminder to fix the commit msgs and merge this PR for @lpm0073 please :)

jfavellar90 commented 1 year ago

@bradenmacdonald @lpm0073 could we just merge https://github.com/openedx/openedx-k8s-harmony/pull/40 before this one? Once https://github.com/openedx/openedx-k8s-harmony/pull/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

bradenmacdonald commented 1 year ago

@jfavellar90 That's fine with me. Thanks for volunteering to do the rebase!

itsjeyd commented 1 year ago

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.

openedx-webhooks commented 1 year ago

@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.