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

Remove workaround for controller-tools typealias bug #6874

Merged

Conversation

tsaarni
Copy link
Member

@tsaarni tsaarni commented Jan 22, 2025

This PR removes the workaround which was applied due to controller-tools issue kubernetes-sigs/controller-tools#1088. The issue was fixed in v0.17.1.

The workaround is not necessary anymore after controller-tools v0.17.1.

Signed-off-by: Tero Saarni <[email protected]>
@tsaarni tsaarni added the release-note/none-required Marks a PR as not requiring a release note. Should only be used for very small changes. label Jan 22, 2025
@tsaarni tsaarni requested a review from a team as a code owner January 22, 2025 07:51
@tsaarni tsaarni requested review from skriss and sunjayBhatia and removed request for a team January 22, 2025 07:51
@sunjayBhatia sunjayBhatia requested review from a team, wilsonwu and davinci26 and removed request for a team January 22, 2025 07:51
Signed-off-by: Tero Saarni <[email protected]>
@tsaarni
Copy link
Member Author

tsaarni commented Jan 22, 2025

Regarding generate-api-docs.sh:

The workaround is still needed in hack/generate-api-docs.sh since following still stands #6709 (comment)

Same problem concerns github.com/ahmetb/gen-crd-api-reference-docs, it also cannot handle inline type alias. The error comes from github.com/kubernetes/gengo which does have some fixes in main. It looked like the behavior is chosen by compiler version used in build, but so far I did not get it to work even if stepping up go version in gen-crd-api-reference-docs go.mod.

The problem there is that we install latest tagged version, which generally makes sense

go install github.com/ahmetb/gen-crd-api-reference-docs

but in this case latest tag is v0.3.0 from year 2021

$ go version -m ~/go/bin/gen-crd-api-reference-docs | grep mod
        mod     github.com/ahmetb/gen-crd-api-reference-docs    v0.3.0  h1:+XfOU14S4bGuwyvCijJwhhBIjYN+YXS18jrCY2EzJaY=

If upgrading to latest (untagged) version with go install github.com/ahmetb/gen-crd-api-reference-docs@latest then generating API docs would work without workaround.

Signed-off-by: Tero Saarni <[email protected]>
Copy link

codecov bot commented Jan 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.05%. Comparing base (3c3fb0a) to head (41ccd07).
Report is 12 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #6874   +/-   ##
=======================================
  Coverage   81.05%   81.05%           
=======================================
  Files         133      133           
  Lines       20026    20026           
=======================================
  Hits        16232    16232           
  Misses       3500     3500           
  Partials      294      294           

@skriss
Copy link
Member

skriss commented Jan 31, 2025

If upgrading to latest (untagged) version with go install github.com/ahmetb/gen-crd-api-reference-docs@latest then generating API docs would work without workaround.

I'd say let's pin it to the latest commit SHA so that it's a stable version so we don't get random changes over time, but I'm fine using an untagged version. Should be able to use go.mod to manage the version and then just go run it from the script.

@tsaarni tsaarni merged commit 27f0923 into projectcontour:main Jan 31, 2025
26 checks passed
@tsaarni
Copy link
Member Author

tsaarni commented Jan 31, 2025

xref #6885

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/none-required Marks a PR as not requiring a release note. Should only be used for very small changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants