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

Add NodeIds in KafkaNodePool printer columns #10287

Merged

Conversation

Vince-Chenal
Copy link
Contributor

Type of change

  • Enhancement

Description

Add a new printer column for KafkaNodePool objects: NodeIds.

Checklist

  • Make sure all tests pass
  • Update CHANGELOG.md

@Vince-Chenal Vince-Chenal force-pushed the kafkanodepool-add-nodeids-column branch from ca4b89f to 435b775 Compare July 1, 2024 08:40
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.

I think the idea seems reasonable assuming it works. But in any case, you would need to build the project to generate the new CRD definitions and not just modify the Java class.

@Vince-Chenal Vince-Chenal force-pushed the kafkanodepool-add-nodeids-column branch from 435b775 to d05c98b Compare July 1, 2024 08:50
@Vince-Chenal
Copy link
Contributor Author

Hey,
Sorry not familiar yet with this operator, I did push the CRDs and I'm currently running tests locally. Thanks 🙏

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.

You will need to do the change tot he CRD also in the packaging/helm/... directory. You can run the make crd_install command to copy it out.

@Vince-Chenal Vince-Chenal force-pushed the kafkanodepool-add-nodeids-column branch from d05c98b to 6205af6 Compare July 1, 2024 08:58
@scholzj scholzj added this to the 0.42.0 milestone Jul 1, 2024
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.

@scholzj scholzj requested a review from ppatierno July 1, 2024 09:51
@scholzj
Copy link
Member

scholzj commented Jul 1, 2024

/azp run acceptance

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Vince-Chenal
Copy link
Contributor Author

I was able to finally run the tests (acceptance+integration), which are running fine locally 👍
Thank you

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. My only concern could be how much the console output will be easily screwed up with a cluster having i.e. 50 brokers and even not using an IDs range from 0 to 49 but i.e. starting from 1000 (just for having more digits). Anyway unless someone will complain, I think it's a nice to have. Thanks for the contribution!

@scholzj scholzj merged commit f4d6a33 into strimzi:main Jul 1, 2024
15 checks passed
@scholzj
Copy link
Member

scholzj commented Jul 1, 2024

Thanks for the PR.

@Vince-Chenal Vince-Chenal deleted the kafkanodepool-add-nodeids-column branch July 1, 2024 16:06
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