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.

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

create consistency around godoc parsing #331

Merged
merged 1 commit into from Feb 28, 2022
Merged

Conversation

uturunku1
Copy link
Contributor

@uturunku1 uturunku1 commented Feb 24, 2022

Description

The goal for this PR is to get the godocs ready for 1.0

There's a bit of inconsistency with the docs, where certain patterns are used for some resources and others deviate from the pattern. To mention a few:

  1. Linking the API docs for the resource when documenting the resource interface
  2. Denoting which fields are required/optional in create, list, update option structs.
  3. Some option structs include docs for each field, others do not
  4. A couple of interfaces have the wrong link to API docs
  5. GoDocs is randomly parsing some of our variables and constants: https://pkg.go.dev/github.com/hashicorp/go-tfe#pkg-constants As you see is only showing the ones defined in organization_membership.go and notification_configuration.go because they just happen to have a comment on top which is what godoc uses to parse and display.

Implementation and rules:

  1. All interfaces must have a comment with a link to the API doc related to it.
  2. All methods for this interface must have a explanation on top of the definition of the method
  3. Helper functions must not have explanation on top of its definition
  4. Variables and functions for internal usage (like the ones on tfe.go) do not need to be documented in GoDoc. This can be avoided by:
  • Having the variables/constants lowercase or prefix with underscore. GoDoc only displays the ones that are uppercase.
  • Adding a space in between the declaration and the comment. GoDoc does not parse comments that are not adjacent to a top-level declaration and will be omitted from godoc’s output

More rules around GoDoc can be found here: https://go.dev/blog/godoc
And their source code can be found here: https://github.com/golang/tools/tree/master/godoc
You can also see how their own source code gets parsed here https://pkg.go.dev/golang.org/x/tools@v0.1.9/godoc

  1. Constants that are typed such as
const (
	Advisory  TaskEnforcementLevel = "advisory"
	Mandatory TaskEnforcementLevel = "mandatory"
)

must be sitting next to the declaration of the type like this:

//TaskEnforcementLevel is an enum that describes the enforcement levels for a run task
type TaskEnforcementLevel string

const (
	Advisory  TaskEnforcementLevel = "advisory"
	Mandatory TaskEnforcementLevel = "mandatory"
)

That way GoDoc can document the type and the constants available to this type as a single group. If they are not sitting next to each other, for example in this case:

//TaskEnforcementLevel is an enum that describes the enforcement levels for a run task
type TaskEnforcementLevel string

const (
	TaskPassed      TaskResultStatus = "passed"
	TaskFailed      TaskResultStatus = "failed"

	Advisory  TaskEnforcementLevel = "advisory"
	Mandatory TaskEnforcementLevel = "mandatory"
)

Then GoDoc will not make the connection between those two and will documented them separately.

Testing plan

These changes were tested by installing godoc locally and running godoc -http=:6060

External links

Output from tests (HashiCorp employees only)

$ TFE_ADDRESS="https://example" TFE_TOKEN="example" TF_ACC="1" go test ./... -v -tags=integration -run TestFunctionsAffectedByChange

...

task_stages_integration_test.go Outdated Show resolved Hide resolved
type SentinelMocksPermissionType string

// WorkspaceLockingPermissionType represents the permissiontype to lock or unlock a workspace.
type WorkspaceLockingPermissionType bool
Copy link
Contributor Author

@uturunku1 uturunku1 Feb 24, 2022

Choose a reason for hiding this comment

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

I deleted this type WorkspaceLockingPermissionType. It was not being used anywhere

@uturunku1 uturunku1 marked this pull request as ready for review Feb 24, 2022
@@ -15,7 +15,7 @@ var _ Plans = (*plans)(nil)
// Plans describes all the plan related methods that the Terraform Enterprise
// API supports.
//
// TFE API docs: https://www.terraform.io/docs/cloud/api/plan.html
// TFE API docs: https://www.terraform.io/cloud-docs/api-docs/plans
Copy link
Contributor Author

@uturunku1 uturunku1 Feb 24, 2022

Choose a reason for hiding this comment

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

FYI, this is not suppose to be plans.html . This would take you to a "page not found"

Copy link
Collaborator

@brandonc brandonc left a comment

👇

// Any organizations with a name or notification email partially matching this value will be returned.
Query string `url:"q,omitempty"`

// Optional:
Copy link
Collaborator

@brandonc brandonc Feb 24, 2022

Choose a reason for hiding this comment

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

Include is an interesting and useful parameter that I think we should take better care to document explicitly. Here's an example from WorkspaceReadOptions:

// Optional: A list of relations to include. See available resources https://www.terraform.io/docs/cloud/api/workspaces.html#available-related-resources

I think we have good docs around which relations are possible to include, and a link would reinforce the relationship we now have to maintain because they are typed strings.

Copy link
Contributor

@sebasslash sebasslash Feb 25, 2022

Choose a reason for hiding this comment

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

☝️

Copy link
Contributor Author

@uturunku1 uturunku1 Feb 25, 2022

Choose a reason for hiding this comment

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

We have a similar comment for the include options in admin_organization.go. You may have not seen it because is in line 75:

// A list of relations to include. See available resources
// https://www.terraform.io/docs/cloud/api/admin/organizations.html#available-related-resources
type AdminOrgIncludeOps string

So, the issue here is that we have discrepancy where we place that comment and link. Most of the time we do it on top of the type and occasionally we do it at the include field definition level, like what you see in workspace.go. I personally like it a bit more on top of the type declaration because Godoc makes types more visible and will include the typed constants for the include field.

@@ -117,6 +117,7 @@ func (s *adminOrganizations) List(ctx context.Context, options *AdminOrganizatio
return orgl, nil
}

// List specific organizations in the Terraform Enterprise installation that have permission to use an organization's modules.
Copy link
Collaborator

@brandonc brandonc Feb 24, 2022

Choose a reason for hiding this comment

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

I think the godoc convention is to begin the comment with the element it describes:

"ListModuleConsumers lists specific organizations..."

Copy link
Contributor

@sebasslash sebasslash Feb 25, 2022

Choose a reason for hiding this comment

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

👆 I noticed there were several comments for functions that don't follow the pattern: // <FUNCTION> does X and Y -- something to keep an eye out for.

Copy link
Contributor Author

@uturunku1 uturunku1 Feb 25, 2022

Choose a reason for hiding this comment

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

I have not ran into that issue before, but I will keep an eye on that.

@uturunku1 uturunku1 force-pushed the update-GoDoc branch 2 times, most recently from 47eccc9 to b6e52e5 Compare Feb 25, 2022
@@ -7,7 +7,7 @@ import (
// Compile-time proof of interface implementation.
var _ TwilioSettings = (*adminTwilioSettings)(nil)

// TwilioSettings describes all the Twilio admin settings.
// TwilioSettings describes all the Twilio admin settings for the Admin Setting API https://www.terraform.io/cloud-docs/api-docs/admin/settings.
Copy link
Contributor

@sebasslash sebasslash Feb 25, 2022

Choose a reason for hiding this comment

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

So I'm curious if we should move the link to a new line for readability.

Copy link
Contributor Author

@uturunku1 uturunku1 Feb 25, 2022

Choose a reason for hiding this comment

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

Will do.

@uturunku1 uturunku1 merged commit c0c4937 into releases-1.0.x Feb 28, 2022
4 checks passed
@uturunku1 uturunku1 deleted the update-GoDoc branch Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants