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

Reuse Util.decodeFromBase64 #9906

Merged
merged 2 commits into from
Apr 3, 2024
Merged

Reuse Util.decodeFromBase64 #9906

merged 2 commits into from
Apr 3, 2024

Conversation

MichaelMorrisEst
Copy link
Contributor

Type of change

Refactoring

Description

Refactor to make use of Util.decodeFromBase64 to reduce code duplication.

Closes #9745

Checklist

Please go through this checklist and make sure all applicable tasks have been done

  • Write tests
  • Make sure all tests pass
  • Update documentation
  • Check RBAC rights for Kubernetes / OpenShift roles
  • Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
  • Reference relevant issue(s) and close them after merging
  • Update CHANGELOG.md
  • Supply screenshots for visual changes, such as Grafana dashboards

Signed-off-by: MichaelMorris <michael.morris@est.tech>
@scholzj
Copy link
Member

scholzj commented Apr 2, 2024

/azp run regression

@scholzj scholzj added this to the 0.41.0 milestone Apr 2, 2024
@scholzj scholzj self-requested a review April 2, 2024 12:51
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@scholzj scholzj requested a review from ppatierno April 2, 2024 12:52
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. The changes look good to me.

Copy link
Contributor

@fvaleri fvaleri left a comment

Choose a reason for hiding this comment

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

Thanks for working on this.

I see that there are few missing replacements in the following classes:

  • KafkaUserModelTest
  • ListenersST

I was also wondering if we should create overloaded methods to cover cases where it takes or return a byte array. Same for the encoding method. This would be a different PR though. Wdyt?

@scholzj
Copy link
Member

scholzj commented Apr 2, 2024

@fvaleri Let's take it step by step to avoid having huge and big PRs full of conflicts and taking a long time to handle.

@fvaleri
Copy link
Contributor

fvaleri commented Apr 2, 2024

@fvaleri Let's take it step by step to avoid having huge and big PRs full of conflicts and taking a long time to handle.

Sure, they could be multiple PRs.

@MichaelMorrisEst
Copy link
Contributor Author

I deliberately excluded the occurrences in KafkaUserModelTest and ListenersST as, unlike the other cases, the charset used in the String constructor is not specified (i.e. the platform's default charset is used)

Perhaps it is ok to use decodeFromBase64(String data) resulting in US_ASCII now being used in these places?

Or perhaps I should:

  • change decodeFromBase64(String data) to not specify the charset in the String constructor, i.e. no longer use US_ASCII by default
  • use that in KafkaUserModelTest and ListenersST, so behaviour is as today
  • use decodeFromBase64(String data, Charset charset) with US_ASCII as charset everywhere decodeFromBase64(String data) is currently used

@scholzj
Copy link
Member

scholzj commented Apr 2, 2024

I think that in KafkaUSerModelTest, US_ASCII should be fine. I think it is the same in the case where a String is created from base64 in ListenersST.

Signed-off-by: MichaelMorris <michael.morris@est.tech>
@scholzj
Copy link
Member

scholzj commented Apr 2, 2024

/azp run regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@fvaleri fvaleri 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.

Copy link
Member

@ppatierno ppatierno 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 PR!

@scholzj scholzj merged commit 44864b8 into strimzi:main Apr 3, 2024
21 checks passed
@MichaelMorrisEst
Copy link
Contributor Author

Thanks for taking the time to review, I can also look at creating issues/PRs for the comment above by @fvaleri on also having methods for byte[], taking it one piece at a time

@scholzj
Copy link
Member

scholzj commented Apr 3, 2024

Thanks for taking the time to review, I can also look at creating issues/PRs for the comment above by @fvaleri on also having methods for byte[], taking it one piece at a time

Sure, feel free to look into it if you want.

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.

[Enhancement]: Use Util.decodeFromBase64 from operator-common module
4 participants