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

Ignore links attribute and print warning message #862

Merged
merged 1 commit into from
Nov 9, 2017

Conversation

xianlubird
Copy link
Contributor

Related to issue #859

Signed-off-by: xianlubird@gmail.com

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 7, 2017
@xianlubird
Copy link
Contributor Author

xianlubird commented Nov 7, 2017

CNCF-CLA Done.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Nov 7, 2017
@surajnarwade
Copy link
Contributor

@xianlubird , please fix functional test examples accordingly

@xianlubird xianlubird force-pushed the feature/ignore-links branch from 8a1b5e9 to fe2b02f Compare November 7, 2017 07:05
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 7, 2017
@xianlubird
Copy link
Contributor Author

@surajnarwade Done, pls review, thanks.

Copy link
Member

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

Thanks for the awesome contribution! :) Left my review!


if f.Name() == "Links" {
//Links has "SERVICE:ALIAS" style, we don't support SERVICE != ALIAS
if linksArray := val.FieldByName(f.Name()); linksArray.Kind() == reflect.Slice {
Copy link
Member

Choose a reason for hiding this comment

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

nested if, may be better to do if f.Name() == "Links" && if linksArray := val.FieldByName(f.Name()); linksArray.Kind() == reflect.Slice {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}
}
}
if !findUnsupportedLinksFlag {
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the code, I don't think this is doing anything? Since findUnsupportedLinksFlag is outside the for i := 0; i < linksArray.Len(); i++ { loop. I could be wrong!

Copy link
Contributor Author

@xianlubird xianlubird Nov 8, 2017

Choose a reason for hiding this comment

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

Thanks for your review @cdrage .

This flag findUnsupportedLinksFlag is for the above code for _, f := range s.Fields() { at about 100 lines this file.The flag's goal is to find compose links like

  links:
    - 'db:mysql'

which service name is different from alias name. If we find links like this, this flag is set to true and code will process, keysFound = append(keysFound, yamlTagName) this code will append links to keysFound and print warning messsage. But if we have links like this

  links:                                links:
    - redis                              - 'mysql:mysql'

There are many compose like this in our examples and test case folder. For this kind of compose ,we can convert it and shouldn't print any warning message. So in this situation, findUnsupportedLinksFlag will set to false, and links will not be append to keysFound. This code keysFound = append(keysFound, yamlTagName) won't run because we have continue, and code will process next filed.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that makes sense. LGTM.

Signed-off-by: xianlubird@gmail.com
@xianlubird xianlubird force-pushed the feature/ignore-links branch from fe2b02f to 9b66188 Compare November 8, 2017 02:08
@surajnarwade
Copy link
Contributor

LGTM

@xianlubird
Copy link
Contributor Author

Thanks for @cdrage @surajnarwade review. So when will this PR be merged ?

@cdrage
Copy link
Member

cdrage commented Nov 9, 2017

LGTM! was just checking that this works locally on my machine. Mergin'

@cdrage cdrage merged commit b6eddd7 into kubernetes:master Nov 9, 2017
@xianlubird xianlubird deleted the feature/ignore-links branch April 27, 2018 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants