Skip to content
This repository has been archived by the owner on Sep 14, 2020. It is now read-only.

Limit persistence annotations to 63 chars #346

Merged
merged 1 commit into from
Apr 16, 2020

Conversation

nolar
Copy link
Contributor

@nolar nolar commented Apr 9, 2020

What do these changes do?

Limit Kopf's annotations' names to 63 characters.

Description

An issue found the hard way (though we hit it before): when an annotation's name is 64 or more characters, it is not accepted by K8s API. This is documented.

For example if we have a prefix, a long function name, and maybe a field suffix: kopf.zalando.org/some_resource_of_interest_created.status.childname — and oops, it is 67 already, while 63 is allowed.

This PR fixes the issue with lengthy handler ids by cutting their tail, and replacing it with some consistent one-way hash — this makes the annotation unique enough, and still recognisable/readable for the primary part of its name.

Issues/PRs

Issues: #331

Type of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • The code addresses only the mentioned problem, and this problem only
  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt

@nolar nolar added the bug Something isn't working label Apr 9, 2020
@zincr
Copy link

zincr bot commented Apr 9, 2020

🤖 zincr found 0 problems , 0 warnings

✅ Large Commits
✅ Approvals
✅ Specification
✅ Dependency Licensing

@zincr
Copy link

zincr bot commented Apr 9, 2020

🤖 zincr found 1 problem , 0 warnings

❌ Approvals
✅ Large Commits
✅ Specification
✅ Dependency Licensing

Details on how to resolve are provided below


Approvals

All proposed changes must be reviewed by project maintainers before they can be merged

Not enough people have approved this pull request - please ensure that 1 additional user, who have not contributed to this pull request approve the changes.

  • ✅ Approved by PR author @nolar
  • ❌ 1 additional approval needed
     

@nolar nolar requested review from 3abdelazim and mnarodovitch April 9, 2020 17:36
@lgtm-com
Copy link

lgtm-com bot commented Apr 9, 2020

This pull request fixes 1 alert when merging 4761ec9 into 698171a - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@stehem
Copy link

stehem commented Apr 16, 2020

👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants