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

Azure support #54

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Azure support #54

wants to merge 1 commit into from

Conversation

parlir
Copy link

@parlir parlir commented Oct 2, 2019

No description provided.

}

fmt.Printf("Starting sleep: %v\n", time.Now().Unix())
time.Sleep(180 * time.Second)
Copy link
Author

Choose a reason for hiding this comment

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

I should probably alter this to just poll on the static IP I provide until it's available. This was a quick hack to fix an issue I had noticed around an agent getting assigned at times +1 builds.

Copy link
Member

Choose a reason for hiding this comment

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

yes this should use polling, which is how we handle this for the other providers. You can use this example as reference https://github.com/drone/autoscaler/blob/master/drivers/amazon/create.go#L122:L175

Msg("cannot terminate instance")
return err
}
return nil
Copy link
Member

Choose a reason for hiding this comment

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

I noticed lines 44-51 and 60-67 look to be the same

Comment on lines +53 to +58
if err != nil {
logger.Error().
Err(err).
Msg("cannot terminate instance")
return err
}
Copy link
Member

Choose a reason for hiding this comment

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

this error is already handled at line 46

Copy link
Author

Choose a reason for hiding this comment

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

Yep will remove

}

// CreatePublicIP creates a new public IP
func (p *provider) createPublicIP(ctx context.Context, ipName string) (ip network.PublicIPAddress, err error) {
Copy link
Member

Choose a reason for hiding this comment

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

is creating a static public IP necessary? can we just use the ephemeral public IP address associated with the instance? This is what we do for other cloud providers.

@@ -40,6 +42,8 @@ type installer struct {
keepaliveTimeout time.Duration
runner config.Runner
labels map[string]string
dockerUsername string
dockerPassword string
Copy link
Member

Choose a reason for hiding this comment

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

these changes can be removed. We have a separate PR that is proposing this change

Copy link
Author

Choose a reason for hiding this comment

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

Yea I proposed that PR 😄 . I'll remove these from this PR


[[override]]
name = "github.com/Azure/go-autorest"
version = "13.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

the Gopkg files were removed and replaced with go.mod files. You may want to sync up your codebase with the latest version

Copy link
Author

Choose a reason for hiding this comment

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

K will move to go.mod

"github.com/drone/autoscaler"
"github.com/rs/zerolog/log"
"fmt"
"time"
Copy link
Member

Choose a reason for hiding this comment

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


func (p *provider) computeMachineRequest(ctx context.Context, base string, opts autoscaler.InstanceCreateOpts) (compute.VirtualMachine, *string, error) {
buf := new(bytes.Buffer)
err := p.userdata.Execute(buf, &opts)
Copy link
Member

Choose a reason for hiding this comment

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

unhandled exception

@bradrydzewski
Copy link
Member

thanks for submitting the pull request. I have added some comments about specific sections of code. I also recommend taking a look at AWS or GCP and trying to more closely mirror the structure and conventions (logging, polling, functions, etc). For example, you will notice we have very detailed logging in the other providers.

*#
*.#*
*~
Copy link
Member

Choose a reason for hiding this comment

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

what are these lines for?

Copy link
Author

Choose a reason for hiding this comment

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

Emacs backup files

Copy link
Member

@bradrydzewski bradrydzewski Oct 16, 2019

Choose a reason for hiding this comment

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

these should be removed and added to your global .gitignore file or in .git/info/exclude

return nil, err
}

azureInstance, err := future.Result(client)
Copy link
Member

Choose a reason for hiding this comment

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

unhandled error

}

azureInstance, err := future.Result(client)
client.Start(ctx, p.resourceGroup, p.vmName)
Copy link
Member

Choose a reason for hiding this comment

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

does start return any parameters? if it returns an error it needs to be handled

return nil, err
}

fmt.Printf("Starting sleep: %v\n", time.Now().Unix())
Copy link
Member

Choose a reason for hiding this comment

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

use logger instead of fmt.Print or println

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants