Update Readme with the optional retry CR parameters #121
Conversation
|
Skipping CI for Draft Pull Request. |
clobrano
force-pushed
the
update-readme-fa-retry-options/0
branch
from
20d985a
to
e6f60dd
Compare
README.md
Outdated
| * `sharedparameters` - common cluster nodes information for executing the fence agent | ||
| * `nodeparameters` - specific cluster nodes information for executing the fence agent |
There was a problem hiding this comment.
Mentioning the next three parameters as optional and not mentioning these two as optional means that they are not optional.
In practice they are optional, even though I don't see a case where a user won't use/set them, thus I am leaning towards setting them as non-optional parameters or simply remove the omitempty. WDYT?
There was a problem hiding this comment.
I guess they are optional just because it's up to the user to decide which specific parameters they want, but you're right that this way is confusing. I might rephrase the last three as "If unset, the default value is..." ?
There was a problem hiding this comment.
it's up to the user to decide which specific parameters they want
and IF they want it. I think it is ok to keep them optional
There was a problem hiding this comment.
"If unset, the default value is..." ?
Or just remove the optional words. Optional parameter, default is "5s". -> Default is "5s".
I don't have a strong preference here.
I think it is ok to keep them optional
Agreed. My question was more about the sharedparameters and nodeparameters which maybe should not be optional. Without them running the fence agent would fail as it misses some parameters.
There was a problem hiding this comment.
I was referring to sharedparameters and nodeparameters too :)
Without them running the fence agent would fail as it misses some parameters
but I didn't realized that. They're surely not optional then
There was a problem hiding this comment.
They're surely not optional then
Yes, I was just looking for another point of view to assert my assumption before taking an action :)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: clobrano, razo7 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:
Approvers can indicate their approval by writing |
clobrano
force-pushed
the
update-readme-fa-retry-options/0
branch
from
e6f60dd
to
bc4f9ec
Compare
clobrano
force-pushed
the
update-readme-fa-retry-options/0
branch
from
bc4f9ec
to
d331b5a
Compare
README.md
Outdated
| The CR includes the following parameters: | ||
|
|
||
| * `agent` - fence-agent name | ||
| * `sharedparameters` - common cluster nodes information for executing the fence agent |
There was a problem hiding this comment.
There was a problem hiding this comment.
It is kind of similar.
I think common cluster nodes information is just saying in different words that it is cluster wide parameters without repeating the word parameters for explaining sharedparameters.
I don't have a strong preference, even though your suggestion might be just more accurate.
There was a problem hiding this comment.
I like @slintes version too, thanks!
Signed-off-by: Carlo Lobrano <[email protected]>
clobrano
force-pushed
the
update-readme-fa-retry-options/0
branch
from
d331b5a
to
6bc2d84
Compare
|
/lgtm |
|
/unhold |
No description provided.