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

feat!: truncate fullname after 22 characters #3248

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

aboguszewski-sumo
Copy link
Contributor

Fixes #3057

The longest name for a statefulset I found was *-otelcloudwatch-logs-collector, the suffix here is 30 characters.

According to kubernetes/kubernetes#64023 the statefulset name should be 52 characters or shorter.

I did not manage to fail rendering the template, I tried using both fail and required function but without success. This is why I'm only truncating the sumologic.fullname name.

Checklist

  • Changelog updated or skip changelog label added
  • Documentation updated
  • Template tests added for new features
  • Integration tests added or modified for major features

@aboguszewski-sumo aboguszewski-sumo requested a review from a team as a code owner September 7, 2023 10:30
@aboguszewski-sumo aboguszewski-sumo added this to the v4.0 milestone Sep 7, 2023
@github-actions github-actions bot added the documentation documentation label Sep 7, 2023
@swiatekm
Copy link

swiatekm commented Sep 7, 2023

I actually think the limit for this wasn't determined by StatefulSets, but by label values. Can you make sure this test:

func TestNameAndLabelLength(t *testing.T) {
also enables otel cloudwatch?

We also need a note about this change in the v4 migration doc.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains invalid labels. Please remove all of the following labels: ['do-not-merge/hold']

@aboguszewski-sumo aboguszewski-sumo force-pushed the ab-fix-release-name-length branch from f669dba to 38d502f Compare September 7, 2023 11:04
@aboguszewski-sumo
Copy link
Contributor Author

Changes applies. But, please note that because this does not cause template rendering to fail, the test actually does not check anything more.

@@ -7,14 +7,15 @@ Expand the name of the chart.

{{/*
Create a default fully qualified app name.
Copy link

Choose a reason for hiding this comment

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

I think we want to make the same change for sumologic.name above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it used anywhere? I grepped for that name and it seems that it's not used anywhere

Copy link

Choose a reason for hiding this comment

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

Huh, we should delete it then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do it in another pr

@aboguszewski-sumo aboguszewski-sumo force-pushed the ab-fix-release-name-length branch from 38d502f to 51cca8f Compare September 7, 2023 11:08
@swiatekm
Copy link

swiatekm commented Sep 7, 2023

Changes applies. But, please note that because this does not cause template rendering to fail, the test actually does not check anything more.

The test checks the actual object names and label values.

@aboguszewski-sumo
Copy link
Contributor Author

What about this then?

// TODO: Add an error check here after we start enforcing the limit: https://github.com/SumoLogic/sumologic-kubernetes-collection/issues/3057

What should we check here if we're not failing the rendering?

@swiatekm
Copy link

swiatekm commented Sep 7, 2023

What about this then?

// TODO: Add an error check here after we start enforcing the limit: https://github.com/SumoLogic/sumologic-kubernetes-collection/issues/3057

What should we check here if we're not failing the rendering?

Maybe just delete that test? We're checking the limits on K8s objects here:

func TestNameAndLabelLength(t *testing.T) {
. Truncating instead of erroring is probably the better choice here.

@swiatekm
Copy link

swiatekm commented Sep 7, 2023

You should also remove the warning I added in #3054. Incidentally, does putting the fail in that file not work?

@aboguszewski-sumo
Copy link
Contributor Author

Incidentally, does putting the fail in that file not work?

in _common.tpl? No, but I only used helm template to test that, maybe an install would fail.

@aboguszewski-sumo
Copy link
Contributor Author

Ok, I tested and fail works when put in NOTES.txt. But we still prefer to truncate, right?

@swiatekm
Copy link

swiatekm commented Sep 7, 2023

Yeah, seems better all around.

@aboguszewski-sumo aboguszewski-sumo force-pushed the ab-fix-release-name-length branch from 76410fa to 51135b6 Compare September 7, 2023 11:57
@aboguszewski-sumo
Copy link
Contributor Author

Warning removed and tests fixed.

releaseName = "collection-test"
releaseName = "col-test"
Copy link

Choose a reason for hiding this comment

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

Why is this change necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Else we get such failures: https://github.com/SumoLogic/sumologic-kubernetes-collection/actions/runs/6109112931/job/16579492782
and I found it easier to modify the constant instead of modifying the tests

Copy link

Choose a reason for hiding this comment

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

But why does this change cause these failures. collection-test is less than 22 characters, so it shouldn't make any difference, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But -sumologic is added to that. And as a result we truncate collection-test-sumologic, which is more than 22 chars.

@swiatekm swiatekm self-requested a review September 7, 2023 13:35
@swiatekm swiatekm force-pushed the ab-fix-release-name-length branch from 51135b6 to 6c65826 Compare September 19, 2023 09:54
@aboguszewski-sumo aboguszewski-sumo merged commit 51cab85 into main Sep 19, 2023
32 checks passed
@aboguszewski-sumo aboguszewski-sumo deleted the ab-fix-release-name-length branch September 19, 2023 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set a limit on the release name
2 participants