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.

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: align kubernetes cluster name validation with EKS #64

Merged
merged 2 commits into from
Nov 19, 2024

Conversation

cartermckinnon
Copy link
Contributor

Issue #, if available:

Description of changes:

This updates the KubernetesClusterName validation to align with EKS, per the eks:CreateCluster docs: https://docs.aws.amazon.com/eks/latest/APIReference/API_CreateCluster.html#API_CreateCluster_RequestSyntax

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

@cartermckinnon cartermckinnon changed the title kubernetes: align cluster name validation with EKS fix: align kubernetes cluster name validation with EKS Nov 12, 2024
@cartermckinnon
Copy link
Contributor Author

Fixed the commitlint check, but the build is failing with a bunch of license errors that don't seem related? https://github.com/bottlerocket-os/bottlerocket-settings-sdk/actions/runs/11803486817/job/32882468268

Copy link
Contributor

@cbgbt cbgbt left a comment

Choose a reason for hiding this comment

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

Looks good to me. I've opened a separate PR to handle the cargo deny failures over here: #65

@cbgbt cbgbt self-requested a review November 13, 2024 00:46
@cbgbt
Copy link
Contributor

cbgbt commented Nov 13, 2024

Ah, I noticed when testing this commit locally that the tests failed, so I ended up pushing my deny.toml update here.

@@ -353,14 +358,14 @@ mod test_kubernetes_cluster_name {

#[test]
fn good_cluster_names() {
for ok in &["more-chars_here.now", &"a".repeat(63)] {
for ok in &["more-chars_here-123", &"a".repeat(100)] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

. no longer being an allowed character is the only notable change in this PR, I think

@cartermckinnon cartermckinnon force-pushed the develop branch 2 times, most recently from 9581fc8 to c0d6639 Compare November 14, 2024 18:59
@cartermckinnon
Copy link
Contributor Author

@cbgbt can you kick off the PR checks?

@cartermckinnon cartermckinnon force-pushed the develop branch 2 times, most recently from 975e203 to 46efab9 Compare November 15, 2024 20:36
@cbgbt cbgbt merged commit bffadea into bottlerocket-os:develop Nov 19, 2024
2 checks passed
Copy link
Contributor

@cbgbt cbgbt left a comment

Choose a reason for hiding this comment

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

I forgot to leave a ✔️ before merging, but this LGTM. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants