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.

Sign up for GitHub

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

Jump to bottom

Add karpenter windows #140

Merged
merged 6 commits into from
Apr 13, 2024
Merged

Add karpenter windows #140

merged 6 commits into from
Apr 13, 2024

Conversation

ROunofF
Copy link
Collaborator

@ROunofF ROunofF commented Sep 5, 2023 •
edited
Loading

Issue #, if available:

Description of changes: Change to use the builder from cdk-eks-blueprints and add a a karpenter addons with a default provisioner for windows.

Blocked by :

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

elamaran11
elamaran11 reviewed Sep 5, 2023
View reviewed changes
Copy link
Contributor

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Approval blocked till next blueprints release.

shapirov103
shapirov103 reviewed Sep 8, 2023
View reviewed changes
Copy link
Contributor

@shapirov103 shapirov103 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice PR, great work! Please see some design/code comments, nothing major.

lib/common/windows-builder.ts Outdated Show resolved Hide resolved
lib/common/windows-builder.ts Outdated Show resolved Hide resolved
@elamaran11
Copy link
Contributor

@ROunofF Could we move this to finish line?

shapirov103
shapirov103 reviewed Sep 19, 2023
View reviewed changes
Copy link
Contributor

@shapirov103 shapirov103 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ROunofF please see my comment

lib/common/windows-builder.ts Outdated Show resolved Hide resolved
@ROunofF ROunofF force-pushed the add-karpenter-windows branch from 73a1aa1 to b143061 Compare September 28, 2023 13:14
@elamaran11
Copy link
Contributor

@ROunofF Please fix the GH Warnings.

@ROunofF ROunofF force-pushed the add-karpenter-windows branch from b143061 to ebab291 Compare December 21, 2023 16:19
@ROunofF
Copy link
Collaborator Author

ROunofF commented Dec 21, 2023

Changed to use cdk-eks-blueprint builder. Remove local builder and some cleanup.

Change to use the eks.nodeGroupOptions so we can override any parameters.

@ROunofF ROunofF marked this pull request as ready for review December 21, 2023 18:57
elamaran11
elamaran11 reviewed Dec 21, 2023
View reviewed changes
Copy link
Contributor

@elamaran11 elamaran11 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ROunofF Some minor feedback. You need check comments on Blueprints repo. Also did you get a chance to fully test by packing WindowsBuilder

lib/windows-construct/index.ts Outdated Show resolved Hide resolved
lib/windows-construct/index.ts Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

This PR has been automatically marked as stale because it has been open 60 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale label Mar 21, 2024
@elamaran11 elamaran11 removed the stale label Mar 21, 2024
@elamaran11
Copy link
Contributor

@ROunofF We had a blueprints release recently. Can you bring this PR to closure.

@ROunofF ROunofF force-pushed the add-karpenter-windows branch from ebab291 to 25e709f Compare April 2, 2024 19:22
elamaran11
elamaran11 reviewed Apr 2, 2024
View reviewed changes
Copy link
Contributor

@elamaran11 elamaran11 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ROunofF Nice work, a minor comment to fix the aws-cdk and aws-cdk-lib version in alignment to blueprints supported version. Did you test it to make sure everything works fine? Once you confirm i can kick in e2e. Also a minor GH Warning to add version to generic-construct

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
elamaran11
elamaran11 reviewed Apr 9, 2024
View reviewed changes
Copy link
Contributor

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ROunofF Everything looks good to me. Did you test it, does the pattern work fine for you? Once #166 is merged, you can merge from main to avoid unnecessary files in PR. Let me run e2e for Windows Pattern.

@elamaran11
Copy link
Contributor

/do-e2e-tests windows deploy

@ROunofF
Copy link
Collaborator Author

ROunofF commented Apr 9, 2024

@ROunofF Everything looks good to me. Did you test it, does the pattern work fine for you? Once #166 is merged, you can merge from main to avoid unnecessary files in PR. Let me run e2e for Windows Pattern.

Yes that pattern was working, had a few problem with unrelated pattern so I used the fix from the PR mentioning it

@shapirov103
Copy link
Contributor

/do-e2e-tests windows deploy

1 similar comment
@shapirov103
Copy link
Contributor

/do-e2e-tests windows deploy

@ROunofF ROunofF force-pushed the add-karpenter-windows branch from e1ee65b to 48cf296 Compare April 12, 2024 13:54
elamaran11
elamaran11 requested changes Apr 12, 2024
View reviewed changes
Copy link
Contributor

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ROunofF Minor comment. Region hard code should be avoided in generic patterns.

lib/karpenter-construct/index.ts Outdated Show resolved Hide resolved
elamaran11
elamaran11 reviewed Apr 12, 2024
View reviewed changes
Copy link
Contributor

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Let me run E2E.

@elamaran11
Copy link
Contributor

/do-e2e-tests windows deploy

@elamaran11
Copy link
Contributor

/do-e2e-tests kubeflow deploy

1 similar comment
@elamaran11
Copy link
Contributor

/do-e2e-tests kubeflow deploy

@elamaran11
Copy link
Contributor

/do-e2e-test windows deploy

@elamaran11
Copy link
Contributor

/do-e2e-test windows destroy

elamaran11
elamaran11 approved these changes Apr 13, 2024
View reviewed changes
Copy link
Contributor

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks for the contribution @ROunofF

@elamaran11 elamaran11 merged commit ea36595 into aws-samples:main Apr 13, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shapirov103 shapirov103 shapirov103 left review comments

@elamaran11 elamaran11 elamaran11 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ROunofF @elamaran11 @shapirov103