Skip to content

Add CRD support to k8s parser#22459

Closed
tim-werner wants to merge 0 commit intopantsbuild:mainfrom
tim-werner:main
Closed

Add CRD support to k8s parser#22459
tim-werner wants to merge 0 commit intopantsbuild:mainfrom
tim-werner:main

Conversation

@tim-werner
Copy link
Copy Markdown
Contributor

This will close issue 22445.
It is now possible to add a custom resource definition to the k8s-parser.

[helm-k8s-parser]
crd = "pantsbuild/crd.py"

In the crd.py file a new CRD has to be defined according to the Hikaru documentation. The base class has to be named CRD.

@tim-werner tim-werner force-pushed the main branch 2 times, most recently from 2524d58 to 66f6715 Compare July 4, 2025 13:41
@huonw huonw requested review from alonsodomin and lilatomic July 14, 2025 02:30
@cburroughs
Copy link
Copy Markdown
Contributor

hi @tim-werner ; thank you the contribution. I've approved the workflow to run. It looks like there are some minor lint/format errors. You will also need to include a (brief) release note.

@tim-werner
Copy link
Copy Markdown
Contributor Author

hi @cburroughs,
thank you very much for the feedback. I have linted the file and have added a brief release note (hopefully to the correct file). Please approve the workflow to run again.
Thx

@alonsodomin
Copy link
Copy Markdown
Contributor

Thanks a lot for this contribution, it does look like something we'd like to have.

A couple of things though:

  • The main objective of the parser is to find the difference container image references. ATM this is totally based on finding out the position of a containers or initContainers entry. If using a CRD that uses different entries then the parser will not be aware of those references. I believe that this should be further explained in the docs (unless a general solution can be implemented)
  • The original ticket uses a list for the extra CRDs but this implementation only accepts a single item. Shouldn't it be a list?

@tim-werner
Copy link
Copy Markdown
Contributor Author

Hi @alonsodomin ,

thank you very much for your comment.

  1. yes you are right that the change should be explained more/better in the docs.
  2. in the original ticket a list is mentioned and it the better (but more complex) solution. However, before I spend more time to implement it as a list I would like to confirm that the general direction of the pull request is fine for you.

@alonsodomin
Copy link
Copy Markdown
Contributor

yeah I like the idea, it's something I knew it would come up someday but avoided getting into that can of worms.

Regarding the list of CRDs, since you also need to know the class name, maybe a Hashmap is a better option.

@tim-werner
Copy link
Copy Markdown
Contributor Author

Thx. I will look into it.

@tim-werner
Copy link
Copy Markdown
Contributor Author

Hi @alonsodomin, @cburroughs ,

I have changed the code in such a way that it is now possible to register multiple crds.

[helm-k8s-parser.crd]
"pantsbuild/crd.py"="CRD"
"pantsbuild/crd_cron2.py"="CRD2"

In addition I have added a section to the Helm Deployment documentation.

Please review.

Thx.

@cburroughs
Copy link
Copy Markdown
Contributor

(hit the CI approval button again)

  • minor lint from ci
  • minor import order conflict
  • @alonsodomin how does the big picture look?

@cburroughs
Copy link
Copy Markdown
Contributor

Thanks for the contribution. We've just branched for 2.29.x, so merging this pull request now will come out in 2.30.x, please move the release notes updates to docs/notes/2.30.x.md if that's appropriate.

@tim-werner
Copy link
Copy Markdown
Contributor Author

Ahh s... I accidently closed the pull request by trying to sync my fork with the main branch...

@tim-werner
Copy link
Copy Markdown
Contributor Author

I have openend a new pull request with the same name and the fixed conflicts. Maybe you can reopen this pull request as a admin. THx.

#22652

cburroughs pushed a commit that referenced this pull request Sep 23, 2025
Accidentaly closed: #22459

---------

Co-authored-by: A. Alonso Dominguez <2269440+alonsodomin@users.noreply.github.com>
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.

3 participants