Skip to content

Create separate documentation.yaml file #447

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

Merged
merged 4 commits into from
May 20, 2023

Conversation

RedbackThomson
Copy link
Contributor

Closes aws-controllers-k8s/community#1223
Expands upon #394

Description of changes:
This pull request moves the custom GoDoc overrides from inside the generator.yaml file to a new, separate documentation.yaml file. This documentation.yaml file currently supports modify field GoDocs (but could be extended to modify resource GoDocs as well) in the following ways:

  • Prepend: Add a comment before the existing shape ref GoDoc
  • Append: Add a comment after the existing shape ref GoDoc
  • Override: Completely replace the existing shape ref GoDoc

The current RDS documentation modifications are now modeled in the documentation.yaml like so:

resources:
  DBClusterParameterGroup:
    fields:
      Parameters: 
        override: |
          DEPRECATED - do not use.  Prefer ParameterOverrides instead.
      ParameterOverrides: 
        append: |
          These are ONLY user-defined parameter overrides for the
          DB cluster parameter group. This does not contain default or system
          parameters.
  DBParameterGroup:
    fields:
      ParameterOverrides: 
        append: |
          These are ONLY user-defined parameter overrides for the DB parameter
          group. This does not contain default or system parameters.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ack-prow ack-prow bot requested review from jljaco and vijtrip2 May 16, 2023 01:26
@ack-prow ack-prow bot added the approved label May 16, 2023
@RedbackThomson RedbackThomson requested review from jaypipes and a-hilaly and removed request for vijtrip2 May 16, 2023 01:26
Copy link
Collaborator

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

great stuff here @RedbackThomson! really appreciate this. Couple smallish nits inline but overall awesome.

Comment on lines 24 to 26
Append *string `json:"append,omitempty"`
Prepend *string `json:"prepend,omitempty"`
Override *string `json:"override,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add docstrings for these fields

// slashes to the beginning of each new line
lines := strings.Split(in, "\n")

// Traverse in reverse order until, deleting empty lines
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason you wanted to delete empty newlines? Sometimes a long docstring is better broken into paragraphs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only deleting trailing newlines here

@jaypipes
Copy link
Collaborator

@RedbackThomson also, see unit test output failure:

 	want (*model.SDKAPI, string, string, "github.com/aws-controllers-k8s/code-generator/pkg/config".Config, "github.com/aws-controllers-k8s/code-generator/pkg/config".DocumentationConfig)
# github.com/aws-controllers-k8s/code-generator/pkg/testutil [github.com/aws-controllers-k8s/code-generator/pkg/generate/ack.test]
pkg/testutil/schema_helper.go:91:73: not enough arguments in call to ackmodel.New
	have (*model.SDKAPI, string, string, "github.com/aws-controllers-k8s/code-generator/pkg/config".Config)
	want (*model.SDKAPI, string, string, "github.com/aws-controllers-k8s/code-generator/pkg/config".Config, "github.com/aws-controllers-k8s/code-generator/pkg/config".DocumentationConfig)
FAIL	github.com/aws-controllers-k8s/code-generator/pkg/generate/ack [build failed]
FAIL	github.com/aws-controllers-k8s/code-generator/pkg/generate/code [build failed]
?   	github.com/aws-controllers-k8s/code-generator/pkg/generate/crossplane	[no test files]
?   	github.com/aws-controllers-k8s/code-generator/pkg/generate/olm	[no test files]
?   	github.com/aws-controllers-k8s/code-generator/pkg/generate/templateset	[no test files]
?   	github.com/aws-controllers-k8s/code-generator/pkg/metadata	[no test files]
FAIL	github.com/aws-controllers-k8s/code-generator/pkg/model [build failed]
FAIL	github.com/aws-controllers-k8s/code-generator/pkg/model/multiversion [build failed]
ok  	github.com/aws-controllers-k8s/code-generator/pkg/sdk	0.319s
make: *** [Makefile:56: test] Error 2 

@RedbackThomson RedbackThomson requested a review from jaypipes May 16, 2023 17:53
@a-hilaly
Copy link
Member

/retest

@RedbackThomson
Copy link
Contributor Author

@a-hilaly @jaypipes Unit tests passing + added code doc

@a-hilaly
Copy link
Member

Nice, ty @RedbackThomson !
/lgtm

@RedbackThomson
Copy link
Contributor Author

This will require a follow up with the RDS controller to move their stuff over. Reminder to me to do that

@a-hilaly
Copy link
Member

/lgtm

1 similar comment
@a-hilaly
Copy link
Member

/lgtm

@ack-prow ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label May 19, 2023
@ack-prow
Copy link

ack-prow bot commented May 19, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, RedbackThomson

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:
  • OWNERS [A-Hilaly,RedbackThomson]

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

@a-hilaly
Copy link
Member

/retest

@ack-prow ack-prow bot merged commit 6542e04 into aws-controllers-k8s:main May 20, 2023
@RedbackThomson RedbackThomson deleted the doc-file branch June 15, 2023 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support custom documentation for fields
3 participants