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

Added ability for calicoctl to read multiple yaml documents in the same file #1680

Merged
merged 1 commit into from
Jul 11, 2017

Conversation

mgleung
Copy link
Contributor

@mgleung mgleung commented Jul 6, 2017

Description

Added the ability for calicoctl to iterate over multiple YAML documents from the same input

Fixes https://github.com/projectcalico/libcalico-go/issues/47

Testing

  • Ran/wrote unit tests
  • Ran system tests
  • Tested by creating multiple policies from the same YAML file locally

Release Note

Added functionality for calicoctl commands to read in multiple yaml documents specified in the same file/input and separated by `---`.

Copy link
Contributor

@bcreane bcreane left a comment

Choose a reason for hiding this comment

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

generally looks good to me. it would be nice if we didn't have to copy/paste the yaml splitter function from k8s, but I don't have a suggestion for how that might be accomplished.

@@ -0,0 +1,81 @@
// Copyright (c) 2016 Tigera, Inc. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

2017?

@@ -0,0 +1,117 @@
// Copyright (c) 2016 Tigera, Inc. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

2017

@tmjd
Copy link
Member

tmjd commented Jul 7, 2017

Please make sure you do a little testing with failures in the yaml docs (invalid fields, bad values in fields) to make sure the error reporting makes sense. From what I've seen before I expect that it will.

@mgleung
Copy link
Contributor Author

mgleung commented Jul 7, 2017

Tested a few misconfigured yaml docs (invalid types in fields, empty fields, wrong options in fields, empty documents) and found that everything makes sense. We could modify the error messages in go-yaml-wrapper so that they are less specific to the underlying resource object and instead reference the field name in the yaml document but those changes would be a part of a separate PR and would also change the existing error messages.

Also forgetting the yaml --- separator in the files only applies the last resource specified in the document which mirrors the current behavior of calicoctl. This could be improved to throw errors, but would also require modifications to the go-yaml library and is outside of the scope of this set of changes.

return nil, err
}

if resources == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, you don't need this first branch - you can append to a nil slice which will create the slice for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good to know

@@ -21,14 +21,15 @@ import (
"reflect"
"strings"

"io/ioutil"
"io"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I realize this was already here, but we should probably just group together all of the native golang imports rather than having them split up across four groups.

@@ -21,14 +21,15 @@ import (
"reflect"
"strings"

"io/ioutil"
"io"
Copy link
Contributor

Choose a reason for hiding this comment

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

Copyright statements in all your updated files should be updated to include 2017.

Copy link
Contributor

@robbrockbank robbrockbank 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 minor nit picks, but otherwise this looks great.

@robbrockbank
Copy link
Contributor

To reiterate Eriks comment - deffo test live with some odd scenarios - e.g. no data between the --- separators, invalid data in one of the documents but not the others etc.

Also, good to test using both file input and stdin.

@mgleung mgleung force-pushed the yaml-multi-doc branch 2 times, most recently from ba52d71 to 7e3302e Compare July 11, 2017 18:33
@caseydavenport caseydavenport added this to the next-milestone milestone Jul 11, 2017
Copy link
Contributor

@robbrockbank robbrockbank left a comment

Choose a reason for hiding this comment

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

Can you rebase off the latest master.

@robbrockbank robbrockbank merged commit 2b6e4a4 into projectcalico:master Jul 11, 2017
@mgleung mgleung deleted the yaml-multi-doc branch July 11, 2017 23:52
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.

5 participants