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

Azure driver: Fix issue #4244 #4245

Merged
merged 1 commit into from Oct 13, 2017

Conversation

Projects
None yet
3 participants
@m4r10k
Contributor

m4r10k commented Sep 6, 2017

This will fix my issue #4244. We are currently using this for our private Gitlab environment to use the auto scaling feature from Gitlab runner with docker-machine and Microsoft Azure.

Signed-off-by: Mario Kleinsasser mario.kleinsasser@gmail.com

@m4r10k m4r10k changed the title from Fix #4244 to Azure driver: Fix issue #4244 Sep 12, 2017

@m4r10k

This comment has been minimized.

Show comment
Hide comment
@m4r10k

m4r10k Sep 13, 2017

Contributor

@shin Would it be possible for you to check why the travis build failed please? Because the error in travis is not clear to me and the build of another patch of mine worked without problems. Thank you!

Contributor

m4r10k commented Sep 13, 2017

@shin Would it be possible for you to check why the travis build failed please? Because the error in travis is not clear to me and the build of another patch of mine worked without problems. Thank you!

@m4r10k

This comment has been minimized.

Show comment
Hide comment
@m4r10k

m4r10k Sep 17, 2017

Contributor

@shin @StefanScherer @nathanleclaire any comments on this from the committers? Is something missing or unclear? Can I do something to get this commited? Thank you!

Contributor

m4r10k commented Sep 17, 2017

@shin @StefanScherer @nathanleclaire any comments on this from the committers? Is something missing or unclear? Can I do something to get this commited? Thank you!

@nathanleclaire

This comment has been minimized.

Show comment
Hide comment
@nathanleclaire
Contributor

nathanleclaire commented Sep 18, 2017

ping @shin-

@m4r10k

This comment has been minimized.

Show comment
Hide comment
@m4r10k

m4r10k Sep 18, 2017

Contributor

Oh, now I see my mistake 😊 ... shin- not shin. 😄 Sorry!

Contributor

m4r10k commented Sep 18, 2017

Oh, now I see my mistake 😊 ... shin- not shin. 😄 Sorry!

@shin-

This comment has been minimized.

Show comment
Hide comment
@shin-

shin- Sep 22, 2017

Member

Hi @kleinsasserm , I believe you need to run a gofmt pass on this PR for the Travis tests to pass.

Member

shin- commented Sep 22, 2017

Hi @kleinsasserm , I believe you need to run a gofmt pass on this PR for the Travis tests to pass.

@m4r10k

This comment has been minimized.

Show comment
Hide comment
@m4r10k

m4r10k Sep 24, 2017

Contributor

@shin- I wrote you an email offline, because I do not spam the PR. I do not get why the Travis build is complaining about lint, but make lint doesn't. Maybe you can have a look in the next days.

Contributor

m4r10k commented Sep 24, 2017

@shin- I wrote you an email offline, because I do not spam the PR. I do not get why the Travis build is complaining about lint, but make lint doesn't. Maybe you can have a look in the next days.

@m4r10k

This comment has been minimized.

Show comment
Hide comment
@m4r10k

m4r10k Sep 28, 2017

Contributor

@shin- Can you have a look at my email? I would like to do this PR the correct way, but I need some help here. Anyone else who can have a look at my PR and my mistake(s) 😊 ?

Contributor

m4r10k commented Sep 28, 2017

@shin- Can you have a look at my email? I would like to do this PR the correct way, but I need some help here. Anyone else who can have a look at my PR and my mistake(s) 😊 ?

@shin-

This comment has been minimized.

Show comment
Hide comment
@shin-

shin- Sep 28, 2017

Member

This is what I'm seeing on Travis in terms of linting errors:

drivers/azure/azure.go:393:2: redundant if ...; err != nil check, just return error instead.
drivers/azure/azure.go:436:2: redundant if ...; err != nil check, just return error instead.
drivers/exoscale/exoscale.go:458:2: redundant if ...; err != nil check, just return error instead.
drivers/hyperv/hyperv.go:180:2: redundant if ...; err != nil check, just return error instead.
drivers/vmwarevcloudair/vcloudair.go:448:2: redundant if ...; err != nil check, just return error instead.
drivers/vmwarevsphere/vsphere.go:938:2: redundant if ...; err != nil check, just return error instead.
libmachine/persist/filestore.go:56:2: redundant if ...; err != nil check, just return error instead.
libmachine/provision/arch.go:150:2: redundant if ...; err != nil check, just return error instead.
libmachine/provision/boot2docker.go:258:2: redundant if ...; err != nil check, just return error instead.
libmachine/provision/coreos.go:130:2: redundant if ...; err != nil check, just return error instead.
libmachine/provision/debian.go:141:2: redundant if ...; err != nil check, just return error instead.
libmachine/provision/rancheros.go:139:2: redundant if ...; err != nil check, just return error instead.
libmachine/provision/redhat.go:113:2: redundant if ...; err != nil check, just return error instead.
libmachine/provision/redhat.go:181:2: redundant if ...; err != nil check, just return error instead.
libmachine/provision/suse.go:205:2: redundant if ...; err != nil check, just return error instead.
libmachine/provision/ubuntu_systemd.go:151:2: redundant if ...; err != nil check, just return error instead.
libmachine/provision/ubuntu_upstart.go:160:2: redundant if ...; err != nil check, just return error instead.
make: *** [lint] Error 1
mk/validate.mk:18: recipe for target 'lint' failed
make: *** [build validate] Error 2

None of those seem related to your changes, so I'm guessing they might be caused by an update of the linter that made it stricter. I'll look into fixing those in master when I have some time. Thank you for your patience.

Member

shin- commented Sep 28, 2017

This is what I'm seeing on Travis in terms of linting errors:

drivers/azure/azure.go:393:2: redundant if ...; err != nil check, just return error instead.
drivers/azure/azure.go:436:2: redundant if ...; err != nil check, just return error instead.
drivers/exoscale/exoscale.go:458:2: redundant if ...; err != nil check, just return error instead.
drivers/hyperv/hyperv.go:180:2: redundant if ...; err != nil check, just return error instead.
drivers/vmwarevcloudair/vcloudair.go:448:2: redundant if ...; err != nil check, just return error instead.
drivers/vmwarevsphere/vsphere.go:938:2: redundant if ...; err != nil check, just return error instead.
libmachine/persist/filestore.go:56:2: redundant if ...; err != nil check, just return error instead.
libmachine/provision/arch.go:150:2: redundant if ...; err != nil check, just return error instead.
libmachine/provision/boot2docker.go:258:2: redundant if ...; err != nil check, just return error instead.
libmachine/provision/coreos.go:130:2: redundant if ...; err != nil check, just return error instead.
libmachine/provision/debian.go:141:2: redundant if ...; err != nil check, just return error instead.
libmachine/provision/rancheros.go:139:2: redundant if ...; err != nil check, just return error instead.
libmachine/provision/redhat.go:113:2: redundant if ...; err != nil check, just return error instead.
libmachine/provision/redhat.go:181:2: redundant if ...; err != nil check, just return error instead.
libmachine/provision/suse.go:205:2: redundant if ...; err != nil check, just return error instead.
libmachine/provision/ubuntu_systemd.go:151:2: redundant if ...; err != nil check, just return error instead.
libmachine/provision/ubuntu_upstart.go:160:2: redundant if ...; err != nil check, just return error instead.
make: *** [lint] Error 1
mk/validate.mk:18: recipe for target 'lint' failed
make: *** [build validate] Error 2

None of those seem related to your changes, so I'm guessing they might be caused by an update of the linter that made it stricter. I'll look into fixing those in master when I have some time. Thank you for your patience.

@m4r10k

This comment has been minimized.

Show comment
Hide comment
@m4r10k

m4r10k Sep 29, 2017

Contributor

You are welcome! I just wanted to make sure that it wasn't my mistake 😄 - if there is anything else I can do for this PR, please let me know.

Contributor

m4r10k commented Sep 29, 2017

You are welcome! I just wanted to make sure that it wasn't my mistake 😄 - if there is anything else I can do for this PR, please let me know.

@shin-

This comment has been minimized.

Show comment
Hide comment
@shin-

shin- Oct 2, 2017

Member

@kleinsasserm I just merged #4266 , so if you can rebase this PR we should be able to move forward. Thank you for your patience! 👍

Member

shin- commented Oct 2, 2017

@kleinsasserm I just merged #4266 , so if you can rebase this PR we should be able to move forward. Thank you for your patience! 👍

@shin- shin- added this to the 0.13.0 milestone Oct 2, 2017

@shin-

I don't think we should rely on the subnet.Name != nil condition to proceed.

Show outdated Hide outdated drivers/azure/azureutil/azureutil.go
Show outdated Hide outdated drivers/azure/azureutil/azureutil.go
Fix #4244
Signed-off-by: Mario Kleinsasser <mario.kleinsasser@gmail.com>

Change fmt

Signed-off-by: Mario Kleinsasser <mario.kleinsasser@gmail.com>

Correct lint

Signed-off-by: Mario Kleinsasser <mario.kleinsasser@gmail.com>

Change the subnet check logic

Signed-off-by: Mario Kleinsasser <mario.kleinsasser@gmail.com>
@m4r10k

This comment has been minimized.

Show comment
Hide comment
@m4r10k

m4r10k Oct 12, 2017

Contributor

@shin- I've updated the PR. Hopefully it fits the needs and I made it the correct way. I think that this version is more informative too but it "feels" a little bit complicated. Should we log the err at this point as log.Warn? I've looked at the code of the other methods but I am not sure. Sorry for the delay, but I was busy in the last days and it took me some time to test this change and to understand the returns in detail. 😃

Contributor

m4r10k commented Oct 12, 2017

@shin- I've updated the PR. Hopefully it fits the needs and I made it the correct way. I think that this version is more informative too but it "feels" a little bit complicated. Should we log the err at this point as log.Warn? I've looked at the code of the other methods but I am not sure. Sorry for the delay, but I was busy in the last days and it took me some time to test this change and to understand the returns in detail. 😃

@shin-

shin- approved these changes Oct 13, 2017

@shin-

This comment has been minimized.

Show comment
Hide comment
@shin-

shin- Oct 13, 2017

Member

Looks pretty good to me! Thanks for following up, and thank you again for your contribution!

Member

shin- commented Oct 13, 2017

Looks pretty good to me! Thanks for following up, and thank you again for your contribution!

@shin- shin- merged commit 83812d4 into docker:master Oct 13, 2017

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
dco-signed All commits are signed
@m4r10k

This comment has been minimized.

Show comment
Hide comment
@m4r10k

m4r10k Oct 13, 2017

Contributor

You are welcome! Thank you!

Contributor

m4r10k commented Oct 13, 2017

You are welcome! Thank you!

@m4r10k m4r10k deleted the m4r10k:4244-fix-azure-delete-rt-entry branch Oct 13, 2017

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