-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fix docs YAML generation setting "kubernetes" for "swarm" #802
Conversation
Due to a copy/paste error, commands annotated with "swarm" were incorrectly setting the "kubernetes" property. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Codecov Report
@@ Coverage Diff @@
## master #802 +/- ##
=========================================
+ Coverage 50.89% 50.9% +<.01%
=========================================
Files 237 237
Lines 15338 15338
=========================================
+ Hits 7807 7808 +1
+ Misses 7029 7028 -1
Partials 502 502 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Easy to miss this kind of thing in a huge PR :)
@@ -123,7 +123,7 @@ func GenYamlCustom(cmd *cobra.Command, w io.Writer) error { | |||
cliDoc.Kubernetes = true | |||
} | |||
if _, ok := curr.Annotations["swarm"]; ok && !cliDoc.Swarm { | |||
cliDoc.Kubernetes = true | |||
cliDoc.Swarm = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something still looks weird here.
It shouldn't need to check !cliDoc.Swarm
because if it's true, setting it to true again doesn't hurt. Is this just an unnecessary statement that can be removed?
I guess all the checks above are the same, so maybe it was just copied?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, was looking at that as well, but didn't want to change without going through the whole logic. I can look at it for a follow-up 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 applied this locally to build the 18.01 CLI refs
author LGTM, let's merge this 👍 |
@andrewhsu this is needed in 18.01 |
Sorry guys... 😕 |
Due to a copy/paste error, commands annotated with "swarm" were incorrectly setting the "kubernetes" property.
This was introduced in f1b1161 (#721)
- Description for the changelog
ping @silvin-lubecki @MistyHacks @dnephin PTAL