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

Adler32 string transform implementation #4417

Merged
merged 3 commits into from
Aug 21, 2023

Conversation

stevendborrelli
Copy link
Contributor

@stevendborrelli stevendborrelli commented Jul 31, 2023

Description of your changes

Adds an Adler-32 checksum, this is used with some Let's Encrypt configurations.

Testing:

I tested the hashing using the following Python script, these values matched the values generated by the transform.

python3
Python 3.11.2 (main, Feb 16 2023, 02:51:42) [Clang 14.0.0 (clang-1400.0.29.202)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import zlib
>>> zlib.adler32("⡌⠁⠧⠑ ⠼⠁⠒  ⡍⠜⠇⠑⠹⠰⠎ ⡣⠕⠌".encode('utf-8'))
4110427190
>>> zlib.adler32(b'Crossplane')
373097499
>>> 

I have:

  • Read and followed Crossplane's contribution process.
  • Added or updated unit and E2E tests for my change.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.
  • Opened a PR updating the docs, if necessary. add doc for adler32 transform docs#524

@stevendborrelli stevendborrelli requested review from a team and muvaf as code owners July 31, 2023 12:44
@stevendborrelli stevendborrelli changed the title initial adler32 implementation Adler32 string transform implementation Jul 31, 2023
},
want: want{
o: "471008351",
//o: "373097499",
Copy link
Contributor

Choose a reason for hiding this comment

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

is this the right value once we get #4445 merged?

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 need to update it to the correct value after rebasing on #4445.

Copy link
Contributor

@pedjak pedjak left a comment

Choose a reason for hiding this comment

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

It would be nice to update the docs as well, pushing a PR against https://github.com/crossplane/docs/

@phisco
Copy link
Contributor

phisco commented Aug 8, 2023

Looks like there are conflicts, @stevendborrelli, could you rebase and address the comment from @pedjak about the doc, we actually added a check to the PRs checklist for that now too 🙏

Signed-off-by: Steven Borrelli <steve@borrelli.org>
Signed-off-by: Steven Borrelli <steve@borrelli.org>
Signed-off-by: Steven Borrelli <steve@borrelli.org>
@stevendborrelli
Copy link
Contributor Author

Pull request has been rebased off of #4445 and the test hashes have been updated to correct values.

@stevendborrelli
Copy link
Contributor Author

Docs PR created: crossplane/docs#524

@stevendborrelli stevendborrelli added the enhancement New feature or request label Aug 8, 2023
@phisco phisco merged commit a888ea7 into crossplane:master Aug 21, 2023
22 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants