Skip to content

Conversation

@humblec
Copy link
Contributor

@humblec humblec commented Jan 15, 2017

#issue #39345
Signed-off-by: Humble Chirammal hchiramm@redhat.com

@k8s-ci-robot
Copy link
Contributor

Hi @humblec. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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 the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 15, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Jan 15, 2017
@humblec
Copy link
Contributor Author

humblec commented Jan 15, 2017

@rootfs PTAL. I havent updated the references ( update-all.sh) in apis. Once this looks good, I will add it as a seperate commit.

@rootfs
Copy link
Contributor

rootfs commented Jan 16, 2017

@k8s-bot ok to test

@humblec humblec force-pushed the iscsi-multipath-backuptp branch from a59dd5a to ff2cea9 Compare January 17, 2017 04:56
@rootfs
Copy link
Contributor

rootfs commented Jan 17, 2017

W0116 21:02:34.943] errors in package "k8s.io/kubernetes/pkg/generated/openapi":
W0116 21:02:34.944] output for "openapi/zz_generated.openapi.go" differs; first existing/expected diff: 
W0116 21:02:34.944]   "iqn\": {\n\t\t\t\t\t\tSchemaProps: spec.SchemaProps{\n\t\t\t\t\t\t\tDescription: \"Target iSCSI Qualified Name.\",\n\t\t\t"
W0116 21:02:34.944]   "backupPortal\": {\n\t\t\t\t\t\tSchemaProps: spec.SchemaProps{\n\t\t\t\t\t\t\tDescription: \"iSCSI target portal List."
W0116 21:02:34.944] 
W0116 21:02:34.975] make[1]: *** [verify_gen_openapi] Error 1
W0116 21:02:34.976] make: *** [verify_generated_files] Error 2
I0116 21:02:35.076] Makefile.generated_files:502: recipe for target 'verify_gen_openapi' failed

@rootfs
Copy link
Contributor

rootfs commented Jan 17, 2017

this needs release note update too,

@humblec humblec force-pushed the iscsi-multipath-backuptp branch from ff2cea9 to 26b2118 Compare January 18, 2017 11:56
@rootfs
Copy link
Contributor

rootfs commented Jan 18, 2017

have you run hack/verify-openapi-spec.sh

@tobad357
Copy link
Contributor

Looking at the logic the iscsi implementation will always try the BackupPortal event if the primary Portal succeeds. This is something which I think is the best option, however if it always uses BackupPortal then it isn't really a Backup.

Wouldn't it be better to have a list for configurations which would be called Portals
If the user configures both Portal and Portals then just append Portal to Portals
Validation should be that either Portal or Portals has been configured

@humblec
Copy link
Contributor Author

humblec commented Jan 19, 2017

@tobad357 Thanks for the comment. The implementation has been done by considering the suggestions made in issue # #21306 . If everyone think for a different name than backupportal, I can definitely change it.

@tobad357
Copy link
Contributor

@humblec When I wrote that issue my thinking was more in line with try the first portal and if it fails then try the backup. As it now is a list of portals and all will be used I vote to call the list "portals" and keep "portal" for backwards compatibility and ease of use

@humblec humblec force-pushed the iscsi-multipath-backuptp branch from 26b2118 to cd360d8 Compare January 19, 2017 13:28
@humblec
Copy link
Contributor Author

humblec commented Jan 19, 2017

@rootfs hack/verify-openapi-spec.sh gives error. Looking into it.

@humblec humblec force-pushed the iscsi-multipath-backuptp branch 2 times, most recently from 3706e8b to 03771ce Compare January 23, 2017 19:58
@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 23, 2017
@rootfs
Copy link
Contributor

rootfs commented Jan 23, 2017

@k8s-bot verify test this

@humblec
Copy link
Contributor Author

humblec commented Jan 23, 2017

now, codecgen is going wrong, checking

@humblec humblec force-pushed the iscsi-multipath-backuptp branch 2 times, most recently from 94158f3 to 2118e9a Compare January 23, 2017 21:35
@humblec humblec force-pushed the iscsi-multipath-backuptp branch from a8bb384 to 97b4748 Compare February 2, 2017 07:55
@k8s-github-robot k8s-github-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 2, 2017
@humblec humblec force-pushed the iscsi-multipath-backuptp branch 2 times, most recently from c625314 to 890c780 Compare February 2, 2017 14:22
@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 2, 2017
@humblec humblec force-pushed the iscsi-multipath-backuptp branch from 890c780 to 7a523da Compare February 2, 2017 14:39
@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 2, 2017
@humblec
Copy link
Contributor Author

humblec commented Feb 2, 2017

@k8s-bot gci gce e2e test this

@humblec
Copy link
Contributor Author

humblec commented Feb 2, 2017

@rootfs can u restore LGTM and also do the needful for getting this merged ?

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 2, 2017
@rootfs
Copy link
Contributor

rootfs commented Feb 2, 2017

@humblec please rebase and i'll lgtm

@lavalamp lavalamp removed their assignment Feb 4, 2017
Signed-off-by: Humble Chirammal <hchiramm@redhat.com>
Signed-off-by: Humble Chirammal <hchiramm@redhat.com>
@humblec humblec force-pushed the iscsi-multipath-backuptp branch from 7a523da to 72f0a52 Compare February 6, 2017 11:52
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2017
@humblec
Copy link
Contributor Author

humblec commented Feb 6, 2017

@rootfs rebased and tests are green. Can you please restore LGTM and provide approved flag ?

@rootfs
Copy link
Contributor

rootfs commented Feb 6, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 6, 2017
@humblec
Copy link
Contributor Author

humblec commented Feb 9, 2017

@rootfs thanks! @childsb can you please put approved flag on this PR ?

@childsb childsb added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 13, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.