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

Issue #3104: Automatically Generate Document for Operator Executable #4112

Merged
merged 2 commits into from
Jun 29, 2021
Merged

Issue #3104: Automatically Generate Document for Operator Executable #4112

merged 2 commits into from
Jun 29, 2021

Conversation

raptorsun
Copy link
Contributor

po-docgen and Makefile are updated to automatically generate Document/operator.md from cmd/operator/main.go, by parsing the definition of flagset in the init() function.

There is a limitation that constants imported from other packages are not resolved into its final literal value, runtime string contatenation using fmt.Sprintf is not evaluated by the doc generator. However, constants in the same file is resolved and constant expressions such as string concatenation is evaluated.

Release Note Template (will be copied)

Automatically Generate Document for Operator Executable.

@raptorsun raptorsun requested a review from a team as a code owner June 18, 2021 07:55
@@ -0,0 +1,46 @@
Usage of Operator
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Usage of Operator
Operator CLI Flags

nit :)

Copy link
Member

Choose a reason for hiding this comment

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

I think we also need the following at the top of the file so it can be included on website (similar to how API.md is included):

---
title: "Operator CLI Flags"
description: "Lists of possible arguments passed to operator executable."
lead: ""
date: 2021-06-18T08:49:31+00:00
draft: false
images: []
menu:
  docs:
    parent: "operator"
weight: 1000
toc: true
---

date can be static for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

modification done as instructed :)

Comment on lines 206 to 208
fmt.Printf("%d-%02d-%02dT%02d:%02d:%02d-00:00\n",
timeNow.Year(), timeNow.Month(), timeNow.Day(),
timeNow.Hour(), timeNow.Minute(), timeNow.Second())
Copy link
Member

Choose a reason for hiding this comment

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

We cannot do a dynamic date as this will always fail in CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I will put date of today hardcoded.

@raptorsun
Copy link
Contributor Author

/retest

@raptorsun
Copy link
Contributor Author

rebase to master branch, hope the previously failed E2E test will pass.

Copy link
Contributor

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

A few comments but good job!

cmd/po-docgen/operator.go Outdated Show resolved Hide resolved
cmd/po-docgen/operator.go Outdated Show resolved Hide resolved
cmd/po-docgen/operator.go Outdated Show resolved Hide resolved

if exprCall.Fun.(*ast.SelectorExpr).X.(*ast.Ident).Name == "flagset" {
var argName, argDefaultValue, argDescription string
if exprCall.Fun.(*ast.SelectorExpr).Sel.Name == "StringVar" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it fail hard if it encounters a flag type that isn't supported by the generator? Also a switch/case would be more idiomatic.

selectorExpr, ok := exprCall.Fun.(*ast.SelectorExpr)
if !ok {
    continue
}

switch selectorExpr.Sel.Name {
    case "StringVar":
    ...
    default:
        // return an error
}


func resolveDurationExpr(expr ast.Expr) string {
var exprIdent *ast.Ident
exprSelector, ok := expr.(*ast.SelectorExpr)
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be a nice place to use type switches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

much simpler than c++ 😀

return exprIdent.Name
}

return "No Identifier to Resolve"
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't it return an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree, returning an error is better. anyone wants to add a new argument should also update the doc generator if needed.


func resolveConstStringExpr(expr ast.Expr) string {

exprBasicLit, ok := expr.(*ast.BasicLit)
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto about type switches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants