Autodocumentation of core/orchestrator_core.go #2

jeromeroussin · 2022-05-27T02:03:16Z

Change description

Smaller PR for autodocumentation

core/orchestrator_core.go

clintonk · 2022-06-08T21:19:47Z

core/orchestrator_core.go

+// Bootstrap initializes the orchestrator.
+// It returns an error if the orchestrator is already bootstrapped.
+// Returns:
+//   - utils.BootstrapError if the orchestrator is already bootstrapped


I'm leery of these specific errors being described here. The meaning of BootstrapError is wrong, and the other two don't exist.

clintonk · 2022-06-08T21:24:17Z

core/orchestrator_core.go

@@ -283,6 +346,16 @@ func (o *TridentOrchestrator) bootstrapStorageClasses(ctx context.Context) error

 // Updates the o.volumes cache with the latest backend data. This function should only edit o.volumes in place to avoid
 // briefly losing track of volumes that do exist.
+// bootstrapVolumes adds existing volumes to the orchestrator


The generator ignored the non-conforming comment here. A better approach might be to preserve the human-created content, not add the generated description, but add the other sections.

Would the existing comments have been recognized as function documentation and formatted as such by godoc? In my code I assumed that if the comment block didn't start with "// function_name" then it was just comments rather than actual doc. I can change that and assume the opposite (and expand with parameters, returns, example)

clintonk · 2022-06-08T21:25:25Z

core/orchestrator_core.go

+// Parameters:
+//    ctx - context for the operation
+// Returns:
+//    error - any error encountered


"any error encountered" ... kinda obvious!

core/orchestrator_core.go

clintonk · 2022-06-08T21:49:07Z

core/orchestrator_core.go

@@ -1884,6 +2244,18 @@ func (o *TridentOrchestrator) CloneVolume(
 	return o.cloneVolumeInitial(ctx, volumeConfig)
 }

+// cloneVolumeInitial creates a new volume by cloning an existing volume.


The AI fails to distinguish the separate roles of cloneVolumeInitial() and cloneVolumeRetry, which are similar to the human-commented addVolumeInitial() and addVolumeRetry(). Deeper insight is needed to get these right, and this is a case where I would overwrite the AI content to add more color.

clintonk · 2022-06-08T21:54:52Z

core/orchestrator_core.go

@@ -2454,6 +2893,17 @@ func (o *TridentOrchestrator) GetVolumeCreatingTransaction(
 	}
 }

+// GetVolumeTransaction returns a volume transaction from the backend


This is simply wrong. The transaction records are kept in the persistent store, not the backends. In fact, the code here and in the called method contains no mention of a backend, so the AI is really off in the weeds here.

core/orchestrator_core.go

clintonk · 2022-06-08T22:03:24Z

core/orchestrator_core.go

@@ -3653,6 +4326,17 @@ func (o *TridentOrchestrator) ListSnapshotsForVolume(
 	return snapshots, nil
 }

+// ReadSnapshotsForVolume returns a list of snapshots for the specified volume


Here is another case where deeper insight would help. The previous method ListSnapshotsForVolume() returns all snapshots cached here in the orchestrator core, while ReadSnapshotsForVolume() invokes storage APIs to get the list of snapshots directly from a storage backend. The comment here isn't wrong, but it could be much more helpful.

core/orchestrator_core.go

ntap-rippy · 2022-06-09T17:52:48Z

core/orchestrator_core.go

+// Parameters:
+//    backendUUID - backend to check
+// Returns:
+//    (capable, error)


Odd that it has 2 on one line with no documentation.

That I think I can address in my script. I am going to verify that openAi generates the expected number of lines under "// Returns"

ntap-rippy · 2022-06-09T17:56:00Z

core/orchestrator_core.go

@@ -4487,6 +5380,17 @@ func (o *TridentOrchestrator) updateBackendOnPersistentStore(
 	return nil
 }

+// updateVolumeOnPersistentStore updates the volume information in persistent store


There's some inconsistency in how it generates this. Sometimes it generates "in the persistent store" and sometimes it leaves off the article "the" and just generates "persistent store".

ntap-rippy · 2022-06-09T17:57:49Z

core/orchestrator_core.go

+// Parameters:
+//   nodeName: name of the node to get the list of VolumePublications for
+// Returns:
+//   []*utils.VolumePublication: list of VolumePublications for the given node


Seems to have gotten confused and listed the type of the return []*utils.VolumePublication, rather than the name publications

Is it possible here that the name is less interesting than the type for the user? The model returned the type quite a few times in autogenerated docs elsewhere in this file.

Whichever, as long as it's consistent

core/orchestrator_core.go

clintonk · 2022-07-11T14:45:47Z

core/orchestrator_core.go

@@ -41,6 +41,17 @@ const (
 // recordTiming is used to record in Prometheus the total time taken for an operation as follows:
 //   defer recordTiming("backend_add")()
 // see also: https://play.golang.org/p/6xRXlhFdqBd
+// It returns an error if the backend update is invalid.


I like that the script can enhance an existing comment. How does that work with subsequent comment regeneration? Can the script still distinguish between manual and autogenerated content?

Currently the whole doc for that function would be detected as "fair play" and possibly rewritten when the hash changes.

Does that mean a regeneration would overwrite the original manually-generated content? That would seem problematic. I wondered if your It returns language was a cue that anything before that was to be preserved, but I see that is frequently used by the AI. What else might work?

It might overwrite the original manual content yes. But possibly that content became obsolete with the code change yes? The human writing the new code would decide whether the generated doc is acceptable. If it isn't they would updated it manually and remove the autogenerated tag?

The hash line starts with // --. If the script placed another delimiter line between preexisting & generated content, it could preserve the former during an update. Thoughts, @ntap-rippy?

core/orchestrator_core.go

clintonk · 2022-07-11T18:45:18Z

core/orchestrator_core.go

@@ -4294,6 +5542,18 @@ func (o *TridentOrchestrator) DeleteNode(ctx context.Context, nodeName string) (
 	return nil
 }

+// deleteNode deletes a node from the backend.


This is a bit misleading. As with the previous function, this one deletes a node from Trident, and it removes access to that node from all backends. We may have to push a commit atop the AI generated content to fix those things it doesn't quite grok.

core/orchestrator_core.go

jeromeroussin added 2 commits May 26, 2022 21:59

Autodocumentation of core/orchestrator_core.go

acc653f

Fixed bug around handling of 'type' and extraneous line after doc

1bd27a1

clintonk reviewed Jun 8, 2022

View reviewed changes

ntap-rippy reviewed Jun 9, 2022

View reviewed changes

core/orchestrator_core.go Show resolved Hide resolved

ntap-rippy reviewed Jun 9, 2022

View reviewed changes

core/orchestrator_core.go Outdated Show resolved Hide resolved

ntap-rippy reviewed Jun 9, 2022

View reviewed changes

core/orchestrator_core.go Outdated Show resolved Hide resolved

Regenerated doc using latest autodoc version

d47fdd1

clintonk reviewed Jul 11, 2022

View reviewed changes

core/orchestrator_core.go Show resolved Hide resolved

clintonk reviewed Jul 11, 2022

View reviewed changes

core/orchestrator_core.go Show resolved Hide resolved

clintonk reviewed Jul 11, 2022

View reviewed changes

core/orchestrator_core.go Show resolved Hide resolved

clintonk reviewed Jul 11, 2022

View reviewed changes

core/orchestrator_core.go Show resolved Hide resolved

clintonk reviewed Jul 11, 2022

View reviewed changes

core/orchestrator_core.go Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Autodocumentation of core/orchestrator_core.go #2

Autodocumentation of core/orchestrator_core.go #2

jeromeroussin commented May 27, 2022

clintonk Jun 8, 2022

clintonk Jun 8, 2022

jeromeroussin Jun 9, 2022 •

edited

Loading

clintonk Jun 8, 2022

clintonk Jun 8, 2022

clintonk Jun 8, 2022

clintonk Jun 8, 2022

ntap-rippy Jun 9, 2022

jeromeroussin Jun 9, 2022

ntap-rippy Jun 9, 2022

ntap-rippy Jun 9, 2022 •

edited

Loading

jeromeroussin Jun 10, 2022

ntap-rippy Jun 11, 2022

clintonk Jul 11, 2022

jeromeroussin Jul 11, 2022

clintonk Jul 11, 2022 •

edited

Loading

jeromeroussin Jul 11, 2022

clintonk Jul 11, 2022

clintonk Jul 11, 2022

Autodocumentation of core/orchestrator_core.go #2

Are you sure you want to change the base?

Autodocumentation of core/orchestrator_core.go #2

Conversation

jeromeroussin commented May 27, 2022

Change description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeromeroussin Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntap-rippy Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintonk Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeromeroussin Jun 9, 2022 •

edited

Loading

ntap-rippy Jun 9, 2022 •

edited

Loading

clintonk Jul 11, 2022 •

edited

Loading