Skip to content

Conversation

@slayer321
Copy link
Contributor

@slayer321 slayer321 commented Aug 5, 2023

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR adds the feature where we can specifiy the start and end tag to the CRDs description which only need to be there in expremental channel CRDs and exclude from standard channel CRDs.

Which issue(s) this PR fixes:

Fixes #2160

Does this PR introduce a user-facing change?:

Excluding experimental guidance in field descriptions from standard CRDs

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 5, 2023
@k8s-ci-robot k8s-ci-robot requested review from bowei and robscott August 5, 2023 11:38
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 5, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @slayer321. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 5, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 5, 2023
@slayer321
Copy link
Contributor Author

slayer321 commented Aug 5, 2023

I raised this PR which already have Routability changes, and after raising as there was a merge conflict I found that the Routability changes where drop. So the changes I made is tested with Routability part and can confirm it works.

Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Thanks @slayer321! This looks good to me, but it would be helpful if you could start by wrapping some examples so we can confirm it works as intended. Here are the main ones I'm thinking of:

  • // * Service (Mesh conformance profile, experimental, ClusterIP Services only)
  • // ParentRefs from a Route to a Service in the same namespace are "producer"
    // routes, which apply default routing rules to inbound connections from
    // any namespace to the Service.
    //
    // ParentRefs from a Route to a Service in a different namespace are
    // "consumer" routes, and these routing rules are only applied to outbound
    // connections originating from the same namespace as the Route, for which
    // the intended destination of the connections are a Service targeted as a
    // ParentRef of the Route.
  • // When the parent resource is a Service, this targets a specific port in the
    // Service spec. When both Port (experimental) and SectionName are specified,
    // the name and port of the selected port must match both specified values.
  • // * Service (Mesh conformance profile, experimental, ClusterIP Services only)
  • // ParentRefs from a Route to a Service in the same namespace are "producer"
    // routes, which apply default routing rules to inbound connections from
    // any namespace to the Service.
    //
    // ParentRefs from a Route to a Service in a different namespace are
    // "consumer" routes, and these routing rules are only applied to outbound
    // connections originating from the same namespace as the Route, for which
    // the intended destination of the connections are a Service targeted as a
    // ParentRef of the Route.

Comment on lines 189 to 190
startTag := "<experimental:description:start>"
endTag := "<experimental:description:end>"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should just copy xml here? (Non-blocking, just an idea)

Suggested change
startTag := "<experimental:description:start>"
endTag := "<experimental:description:end>"
startTag := "<experimental:description>"
endTag := "</experimental:description>"

Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Thanks @slayer321! Sorry I lost track of this one!

// Gateway has the AllowedRoutes field, and ReferenceGrant provides a
// generic way to enable any other kind of cross-namespace reference.
//
//<experimental:description>
Copy link
Member

Choose a reason for hiding this comment

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

One tiny nit, probably best to stick with the <gateway:experimental prefix we're using elsewhere.

Suggested change
//<experimental:description>
// <gateway:experimental:description>

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 18, 2023
@robscott
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 18, 2023
@shaneutt shaneutt added this to the v1.0.0 milestone Aug 21, 2023
Signed-off-by: slayer321 <[email protected]>
Signed-off-by: slayer321 <[email protected]>
@slayer321
Copy link
Contributor Author

@robscott I have made the changes can you please take a look.

@robscott robscott added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 18, 2023
@robscott
Copy link
Member

Thanks @slayer321, Sorry I lost track of this one! Please add a release-note in the PR description above, maybe something like "Excluding experimental guidance in field descriptions from standard CRDs". Once you do that, I believe the PR should merge.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 18, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: robscott, slayer321

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

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 19, 2023
@k8s-ci-robot k8s-ci-robot merged commit ea382e7 into kubernetes-sigs:main Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.

Projects

No open projects

Development

Successfully merging this pull request may close these issues.

CRD Generation: Exclude Experimental Guidance from Standard Channel CRDs

4 participants