Skip to content

Disable APIExport VirtualWorkspace URL population #3411

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 3 commits into from
Jun 4, 2025
Merged

Disable APIExport VirtualWorkspace URL population #3411

merged 3 commits into from
Jun 4, 2025

Conversation

mjudeikis
Copy link
Contributor

@mjudeikis mjudeikis commented May 18, 2025 •
edited
Loading

Summary

Disable APIExport URL population.

What Type of PR Is This?

/kind feature
/kind deprecation

Related Issue(s)

Fixes #3366

Release Notes

Deprecated APIExport VirtualWorksapce URL population
Add FeatureFlag to re-enable deprecated APIExport VirtualWorkspace URLs.

@kcp-ci-bot kcp-ci-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. dco-signoff: yes Indicates the PR's author has signed the DCO. labels May 18, 2025
@kcp-ci-bot
Copy link
Contributor

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/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 18, 2025
@mjudeikis mjudeikis force-pushed the mjudeikis/disable.VW.url branch from dcd5f3c to ddbde29 Compare May 18, 2025 13:27
@mjudeikis
Copy link
Contributor Author

/test all

@mjudeikis mjudeikis force-pushed the mjudeikis/disable.VW.url branch from ddbde29 to 241ce5c Compare May 18, 2025 13:47
@mjudeikis
Copy link
Contributor Author

/test all

1 similar comment
@mjudeikis
Copy link
Contributor Author

/test all

@mjudeikis mjudeikis force-pushed the mjudeikis/disable.VW.url branch from 82a0632 to ef8f163 Compare May 19, 2025 18:58
@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/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 19, 2025
@mjudeikis
Copy link
Contributor Author

/test all

@mjudeikis mjudeikis force-pushed the mjudeikis/disable.VW.url branch from ef8f163 to eaa52a9 Compare May 19, 2025 19:05
@kcp-ci-bot kcp-ci-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 19, 2025
@mjudeikis mjudeikis marked this pull request as ready for review May 19, 2025 19:07
@kcp-ci-bot kcp-ci-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 19, 2025
@mjudeikis mjudeikis changed the title Add FF to disable URL population. WIP: Add FF to disable URL population. May 19, 2025
@kcp-ci-bot kcp-ci-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 19, 2025
@mjudeikis mjudeikis force-pushed the mjudeikis/disable.VW.url branch 4 times, most recently from 25557c4 to 73128cb Compare May 20, 2025 19:39
@mjudeikis mjudeikis force-pushed the mjudeikis/disable.VW.url branch from d347404 to 471adde Compare May 26, 2025 13:13
@kcp-ci-bot kcp-ci-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2025
@mjudeikis mjudeikis force-pushed the mjudeikis/disable.VW.url branch from 471adde to 2bba901 Compare May 27, 2025 05:11
@kcp-ci-bot kcp-ci-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 27, 2025
@mjudeikis mjudeikis changed the title Disable APIExport VirtualWorkspace.url population Disable APIExport VirtualWorkspace URL population May 27, 2025
@mjudeikis
Copy link
Contributor Author

/retest

@mjudeikis mjudeikis marked this pull request as draft May 27, 2025 11:31
@kcp-ci-bot kcp-ci-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 27, 2025
@mjudeikis
Copy link
Contributor Author

mjudeikis commented May 27, 2025 •
edited
Loading

TODO: add owner reference
Done

@mjudeikis mjudeikis force-pushed the mjudeikis/disable.VW.url branch from 69f213b to 32c7fef Compare May 28, 2025 11:14
@mjudeikis mjudeikis marked this pull request as ready for review May 28, 2025 11:14
@kcp-ci-bot kcp-ci-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 28, 2025
@gman0
Copy link
Contributor

gman0 commented May 29, 2025 •
edited
Loading

The APIExportEndpointSlice is automatically created when an APIExport is created. Shouldn't it be also cleaned up once the APIExport is gone?

@embik
Copy link
Member

embik commented May 30, 2025

The APIExportEndpointSlice is automatically created when an APIExport is created. Shouldn't it be also cleaned up once the APIExport is gone?

There is an owner reference on the APIExportEndpointSlice created in this PR, so deleting the APIExport should subsequently clean up owned objects like this one.

@mjudeikis
Copy link
Contributor Author

/retest

@mjudeikis mjudeikis force-pushed the mjudeikis/disable.VW.url branch from 32c7fef to 4a4809b Compare May 30, 2025 12:47
embik
embik reviewed Jun 2, 2025
View reviewed changes
Copy link
Member

@embik embik left a comment

Choose a reason for hiding this comment

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

Two minor things.

pkg/reconciler/apis/apiexport/apiexport_reconcile.go
if err := c.updateVirtualWorkspaceURLs(ctx, apiExport); err != nil {
conditions.MarkFalse(
apiExport,
apisv1alpha2.APIExportVirtualWorkspaceURLsReady,
Copy link
Member

Choose a reason for hiding this comment

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

Question: What happens to this condition when the feature gate is disabled (i.e. the default from the next release)? Should we clean this up somehow? Otherwise we will have a dangling condition on APIExports from previous versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call. I think we should remove this in else statment

Copy link
Member

Choose a reason for hiding this comment

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

On a similar note, what do we do about APIExports that have the virtual workspace URL set? Should we maybe also wipe them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated with both

@mjudeikis mjudeikis force-pushed the mjudeikis/disable.VW.url branch from c134635 to fd0b98b Compare June 3, 2025 12:13
@mjudeikis mjudeikis mentioned this pull request Jun 3, 2025
Merged
embik
embik approved these changes Jun 4, 2025
View reviewed changes
Copy link
Member

@embik embik left a comment

Choose a reason for hiding this comment

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

/approve

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

LGTM label has been added.

Git tree hash: a60fcfefb417cc8eed222612aba43db289555f62

@kcp-ci-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: embik

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 Jun 4, 2025
@kcp-ci-bot kcp-ci-bot merged commit a44c196 into kcp-dev:main Jun 4, 2025
14 checks passed
@kcp-ci-bot kcp-ci-bot added this to the v0.28.0 milestone Jun 4, 2025
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

@embik embik embik approved these changes

Assignees

@embik embik

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 kind/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. kind/feature Categorizes issue or PR as related to a new feature. 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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v0.28.0
Development

Successfully merging this pull request may close these issues.

feature: disable APIExport endpoints urls for v1alpha2
5 participants
@mjudeikis @kcp-ci-bot @sttts @gman0 @embik