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.

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

[WIP] Add descriptions to AC's CRD's fields #17432

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[WIP] Add descriptions to AC's CRD's fields #17432

wants to merge 4 commits into from

Conversation

VOID404
Copy link
Contributor

@VOID404 VOID404 commented Apr 28, 2023

Description

Changes proposed in this pull request:

  • Add description to AC's CRD's fields
  • Add AC's CRD to doc generation system
  • Prepare the AC's CRD's docs for generation

Related issue(s)

Part of kyma-project/application-connector-manager#155

@VOID404 VOID404 requested review from a team as code owners April 28, 2023 10:49
@netlify
Copy link

netlify bot commented Apr 28, 2023 •
edited
Loading

✅ Deploy Preview for kyma-project-docs-preview ready!

Name Link
🔨 Latest commit fd90c8f
🔍 Latest deploy log https://app.netlify.com/sites/kyma-project-docs-preview/deploys/644ba4cc372dbe0008ecfc94
😎 Deploy Preview https://deploy-preview-17432--kyma-project-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@kyma-bot kyma-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 28, 2023
@VOID404 VOID404 mentioned this pull request Jan 29, 2024
Open
6 tasks
@VOID404 VOID404 changed the title Add descriptions to AC's CRD's fields [WIP] Add descriptions to AC's CRD's fields Apr 28, 2023
@kyma-bot kyma-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 28, 2023
@VOID404
Copy link
Contributor Author

VOID404 commented May 2, 2023 •
edited
Loading

  • both accessLabel are marked deprecated in go source code. Can they be removed?
  • can we just refer to the docs for all the services.entries.credentials?

Not documented:

  • services.entries.apiType
  • compassMetadata
  • displayName
  • providerDisplayName
  • group
  • skipInstallation
  • tenant

akgalwas
akgalwas reviewed May 11, 2023
View reviewed changes
docs/05-technical-reference/00-custom-resources/ac-01-application.md
<!-- Application v1alpha1 applicationconnector.kyma-project.io -->
| Parameter | Description |
| ---------------------------------------- | ---------|
| **spec.accessLabel** | |
Copy link
Contributor

Choose a reason for hiding this comment

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

accessLabel was used for Service Catalog integration, and is no longer relevant. I think we can get rid of that.

docs/05-technical-reference/00-custom-resources/ac-01-application.md
| Parameter | Description |
| ---------------------------------------- | ---------|
| **spec.accessLabel** | |
| **spec.compassMetadata** | |
Copy link
Contributor

Choose a reason for hiding this comment

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

compassMetadata is set by the Compass Runtime Agent. If the property is non-nil it means that the Application was created from Compass Application. If the field is empty, we can assume that Application was created manually, and is not controlled by the Compass Runtime Agent.

docs/05-technical-reference/00-custom-resources/ac-01-application.md
| ---------------------------------------- | ---------|
| **spec.accessLabel** | |
| **spec.compassMetadata** | |
| **spec.compassMetadata.applicationId** | |
Copy link
Contributor

Choose a reason for hiding this comment

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

ApplicationID of the source Compass Application that was used to create Kyma Application

docs/05-technical-reference/00-custom-resources/ac-01-application.md
| **spec.compassMetadata** | |
| **spec.compassMetadata.applicationId** | |
| **spec.compassMetadata.authentication** | |
| **spec.compassMetadata.authentication.clientIds** | |
Copy link
Contributor

Choose a reason for hiding this comment

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

clientIds contains an identifier fetched from Compass that is used for authentication. This field is set by Compass Runtime Agent when Application is created, and used by the Connectivity Validator to determine whether incoming request should be authorised or not. Connectivity Validator expects that the CNAME of the client certificate matches clientId

docs/05-technical-reference/00-custom-resources/ac-01-application.md
| **spec.compassMetadata.authentication** | |
| **spec.compassMetadata.authentication.clientIds** | |
| **spec.description** | Describes the connected Application. |
| **spec.displayName** | |
Copy link
Contributor

Choose a reason for hiding this comment

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

displayName is left empty by the Compass Runtime Agent, so that I would assume it is deprecated.

docs/05-technical-reference/00-custom-resources/ac-01-application.md
| **spec.group** | |
| **spec.labels** | Defines the labels of the Application. |
| **spec.longDescription** | |
| **spec.providerDisplayName** | |
Copy link
Contributor

Choose a reason for hiding this comment

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

providerDisplayName is still set by the Compass Runtime Agent, but I suppose it could be removed. Needs some short investigation.

docs/05-technical-reference/00-custom-resources/ac-01-application.md
| **spec.services.entries.apiType** | |
| **spec.services.entries.centralGatewayUrl** | Specifies the URL of Application Gateway. Internal address resolvable only within the cluster. This field is required for the API entry type. In the [Compass mode](../../01-overview/main-areas/application-connectivity/README.md), it's provided by Runtime Agent. In the [standalone mode](../../01-overview/main-areas/application-connectivity/README.md), you have to provide it yourself.
|
| **spec.services.entries.credentials** | |
Copy link
Contributor

Choose a reason for hiding this comment

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

We could link the tutorials, but I think we should also describe those fields one by one.

docs/05-technical-reference/00-custom-resources/ac-01-application.md
| **spec.services.entries.credentials.type** | |
| **spec.services.entries.gatewayUrl** | |
| **spec.services.entries.id** | |
| **spec.services.entries.name** | New fields used by V2 version |
Copy link
Contributor

Choose a reason for hiding this comment

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

New fields used by V2 version conveys zero information. Entry.name allows to support API Bundles functionality provided by Compass. APIBundle is a set of APIs that share the same credentials. The API Bundle have both name and identifier that are used by the Compass Runtime Agent to set spec.services.name field. Each API in the bundle has name that is used by the Compass Runtime Agent to set spec.services.entries.name.

docs/05-technical-reference/00-custom-resources/ac-01-application.md
| **spec.services.entries.id** | |
| **spec.services.entries.name** | New fields used by V2 version |
| **spec.services.entries.requestParametersSecretName** | |
| **spec.services.entries.specificationUrl** | |
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is deprecated.

docs/05-technical-reference/00-custom-resources/ac-01-application.md
|
| **spec.services.providerDisplayName** | Specifies a human-readable name of the Application service provider. In the [Compass mode](../../01-overview/main-areas/application-connectivity/README.md), it's provided by Runtime Agent. In the [standalone mode](../../01-overview/main-areas/application-connectivity/README.md), you have to provide it yourself.
|
| **spec.services.tags** | Specifies additional tags used for better documentation of the available APIs. In the [Compass mode](../../01-overview/main-areas/application-connectivity/README.md), it's provided by Runtime Agent. In the [standalone mode](../../01-overview/main-areas/application-connectivity/README.md), you have to provide it yourself.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is deprecated, and should be removed.

@Disper Disper mentioned this pull request May 31, 2023
Closed
5 tasks
@kyma-bot
Copy link
Contributor

@VOID404: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pre-main-kyma-gardener-gcp-eventing-upgrade fd90c8f link true /test pre-main-kyma-gardener-gcp-eventing-upgrade
pre-main-serverless-git-auth-integration-k3s fd90c8f link true /test pre-main-serverless-git-auth-integration-k3s
pull-kyma-pjconfigtest fd90c8f link true /test pull-kyma-pjconfigtest
pre-main-kyma-integration-k3d-runtime-agent fd90c8f link true /test pre-main-kyma-integration-k3d-runtime-agent
pull-kyma-governance fd90c8f link true /test pull-kyma-governance
pull-kyma-checkconfig fd90c8f link true /test pull-kyma-checkconfig
pre-main-kyma-integration-k3d-app-gateway fd90c8f link true /test pre-main-kyma-integration-k3d-app-gateway
pre-main-kyma-integration-k3d-app-conn-validator fd90c8f link true /test pre-main-kyma-integration-k3d-app-conn-validator

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@kyma-bot
Copy link
Contributor

This issue or PR has been automatically marked as stale due to the lack of recent activity.
Thank you for your contributions.

This bot triages issues and PRs according to the following rules:

  • After 60d of inactivity, lifecycle/stale is applied
  • After 7d of inactivity since lifecycle/stale was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Close this issue or PR with /close

If you think that I work incorrectly, kindly raise an issue with the problem.

/lifecycle stale

@kyma-bot kyma-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 24, 2023
@Disper Disper removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 25, 2023
@kyma-bot
Copy link
Contributor

This issue or PR has been automatically marked as stale due to the lack of recent activity.
Thank you for your contributions.

This bot triages issues and PRs according to the following rules:

  • After 60d of inactivity, lifecycle/stale is applied
  • After 7d of inactivity since lifecycle/stale was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Close this issue or PR with /close

If you think that I work incorrectly, kindly raise an issue with the problem.

/lifecycle stale

@kyma-bot kyma-bot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. cla: yes Indicates the PR's author has signed the CLA. labels Nov 24, 2023
@VOID404 VOID404 added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akgalwas akgalwas akgalwas left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@akgalwas akgalwas

Labels
cla: yes Indicates the PR's author has signed the CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@VOID404 @kyma-bot @akgalwas @Disper