Skip to content

✨ create apis/v1alpha2, add new ResourceSchemas #3318

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Apr 9, 2025
Merged

✨ create apis/v1alpha2, add new ResourceSchemas #3318

merged 15 commits into from
Apr 9, 2025

Conversation

@xrstf
Copy link
Contributor

@xrstf xrstf commented Mar 3, 2025 •
edited by mjudeikis
Loading

Summary

This PR adds a new version to the apis.kcp.io API, v1alpha2. This new version only contains the slightly updated APIExports, which now support a more elaborate configuration structure to enable virtual resources in the future.

All the code was updated to refer to v1alpha2.APIExports.

Conversion between the two versions is handled by a wrapper around the stock CR Converter, see individual commits for more information.

Related issue(s)

Fixes #3301

TODO:

  • Add admission to check valid json annotations
  • Add admission to check for duplicates

Confirmed that if you add a new type into a new API and don't sort converters, all stuff just explodes in tests and generation :) which is good!

Release Notes

Introduce `apis.kcp.io/v1alpha2`, containing solely `APIExport`

@kcp-ci-bot kcp-ci-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has signed the DCO. labels Mar 3, 2025
@kcp-ci-bot
Copy link
Contributor

kcp-ci-bot commented Mar 3, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@kcp-ci-bot kcp-ci-bot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 3, 2025
@xrstf
Copy link
Contributor Author

xrstf commented Mar 3, 2025

/test all

mjudeikis
mjudeikis reviewed Mar 3, 2025
View reviewed changes
Copy link
Contributor

@mjudeikis mjudeikis 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 few things are missing here:

  1. Conversion webhook for v1alpha1 to v1alpha2
  2. I would really like it to be split in commits:
    All API changes and webhook into 1 commit
    Sed replace changes to another.

But Im in the same boat as you for the approach. @sttts - we need your guidance here :)

@xrstf
Copy link
Contributor Author

xrstf commented Mar 3, 2025

I would really like it to be split in commits: All API changes and webhook into 1 commit, Sed replace changes to another.

Will do, for rebasing it was just simpler to keep it locally as 1 commit for now.

@xrstf xrstf force-pushed the resourceschemas branch 3 times, most recently from 05c99d7 to ba86ec7 Compare March 7, 2025 10:04
@xrstf
Copy link
Contributor Author

xrstf commented Mar 7, 2025

/test all

@kcp-ci-bot kcp-ci-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 10, 2025
@xrstf xrstf force-pushed the resourceschemas branch from ba86ec7 to be360df Compare March 10, 2025 19:41
@kcp-ci-bot kcp-ci-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 10, 2025
@xrstf
Copy link
Contributor Author

xrstf commented Mar 10, 2025

/test all

sttts
sttts reviewed Mar 10, 2025
View reviewed changes
sttts
sttts reviewed Mar 10, 2025
View reviewed changes
sttts
sttts reviewed Mar 10, 2025
View reviewed changes
sttts
sttts reviewed Mar 10, 2025
View reviewed changes
sttts
sttts reviewed Mar 10, 2025
View reviewed changes
@xrstf xrstf force-pushed the resourceschemas branch 2 times, most recently from bbc80c8 to b6aabe3 Compare March 11, 2025 15:59
@xrstf
Copy link
Contributor Author

xrstf commented Mar 11, 2025

/test all

@xrstf xrstf force-pushed the resourceschemas branch from b6aabe3 to c36184f Compare March 11, 2025 19:00
@kcp-ci-bot kcp-ci-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Mar 11, 2025
@xrstf
Copy link
Contributor Author

xrstf commented Mar 11, 2025

/test all

@xrstf xrstf marked this pull request as ready for review March 11, 2025 19:24
@mjudeikis
Copy link
Contributor

mjudeikis commented Apr 6, 2025

/retest

@mjudeikis
Copy link
Contributor

mjudeikis commented Apr 6, 2025

Most issues addressed. Just checkiner where I broke restmapper :/

@mjudeikis
Copy link
Contributor

mjudeikis commented Apr 6, 2025

/retest

@mjudeikis
Copy link
Contributor

mjudeikis commented Apr 6, 2025

/retest
Somehow can't replicate locally :/

@mjudeikis mjudeikis force-pushed the resourceschemas branch 2 times, most recently from 5e85e28 to 2646a55 Compare April 7, 2025 19:08
@mjudeikis
Copy link
Contributor

mjudeikis commented Apr 7, 2025

/hold
want to test one nit before merging, but should be good'ish now.

@kcp-ci-bot kcp-ci-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 7, 2025
sttts
sttts reviewed Apr 7, 2025
View reviewed changes
@mjudeikis
Copy link
Contributor

mjudeikis commented Apr 8, 2025

/retest

@mjudeikis
Copy link
Contributor

mjudeikis commented Apr 8, 2025

/hold cancel

@kcp-ci-bot kcp-ci-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 8, 2025
@sttts
Copy link
Member

sttts commented Apr 9, 2025

/approve

@kcp-ci-bot
Copy link
Contributor

kcp-ci-bot commented Apr 9, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kcp-ci-bot kcp-ci-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 9, 2025
@mjudeikis mjudeikis requested a review from Copilot April 9, 2025 07:56
Copilot AI reviewed Apr 9, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 142 out of 144 changed files in this pull request and generated 1 comment.

Files not reviewed (2)
  • config/crds/apis.kcp.io_apiexports.yaml-patch: Language not supported
  • hack/update-codegen-clients.sh: Language not supported
Comments suppressed due to low confidence (1)

pkg/reconciler/apis/apibinding/apibinding_indexes.go:30

  • The comment still refers to spec.latestResourceSchemas while the code now uses spec.Resources. Please update the comment to accurately reflect the new field.
// indexAPIExportsByAPIResourceSchemasFunc is an index function that maps an APIExport to its spec.latestResourceSchemas.

@xrstf
Copy link
Contributor Author

xrstf commented Apr 9, 2025

/lgtm

@kcp-ci-bot
Copy link
Contributor

kcp-ci-bot commented Apr 9, 2025

@xrstf: you cannot LGTM your own PR.

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@mjudeikis
Copy link
Contributor

mjudeikis commented Apr 9, 2025

/lgtm

@kcp-ci-bot kcp-ci-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2025
@kcp-ci-bot
Copy link
Contributor

kcp-ci-bot commented Apr 9, 2025

LGTM label has been added.

Git tree hash: 3e878f3532912ef1c055a4c1be28abac96722608

@kcp-ci-bot kcp-ci-bot merged commit e72a042 into kcp-dev:main Apr 9, 2025
14 checks passed
@xmudrii xmudrii mentioned this pull request Apr 25, 2025
Merged
@xrstf xrstf deleted the resourceschemas branch May 15, 2025 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sttts sttts sttts left review comments

Copilot code review Copilot Copilot left review comments

@mjudeikis mjudeikis Awaiting requested review from mjudeikis

Assignees

@mjudeikis mjudeikis

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has signed the DCO. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

feature: Implement API change for APIExport latestResourceSchemas

4 participants

@xrstf @kcp-ci-bot @mjudeikis @sttts