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

refactor: worker startup process #68

Merged
merged 3 commits into from
Mar 23, 2020
Merged

refactor: worker startup process #68

merged 3 commits into from
Mar 23, 2020

Conversation

jbrockopp
Copy link
Contributor

@jbrockopp jbrockopp commented Mar 23, 2020

I fixed some of the linter errors but the linter is still failing for a couple checks 😢

The failed check for the gosimple linter (S1000 error) I believe is a bug:

dominikh/go-tools#503

The failed check for the unused linter is a part of my ongoing refactor work but I'm trying to keep this PR a bit smaller.

I vote we ignore the checks 👍

@jbrockopp jbrockopp added the enhancement Indicates an improvement to a feature label Mar 23, 2020
@jbrockopp jbrockopp requested a review from a team as a code owner March 23, 2020 14:38
@jbrockopp jbrockopp self-assigned this Mar 23, 2020
@codecov
Copy link

codecov bot commented Mar 23, 2020

Codecov Report

Merging #68 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #68   +/-   ##
=======================================
  Coverage   64.53%   64.53%           
=======================================
  Files          17       17           
  Lines         953      953           
=======================================
  Hits          615      615           
  Misses        223      223           
  Partials      115      115
Impacted Files Coverage Δ
executor/linux/service.go 58.55% <100%> (ø) ⬆️
executor/linux/step.go 62.04% <100%> (ø) ⬆️

)

// helper function to setup the queue from the CLI arguments.
func setupQueue(c *cli.Context) (queue.Service, error) {
func setupQueue(q *queueSetup) (queue.Service, error) {

Choose a reason for hiding this comment

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

func setupQueue is unused (from unused)

}
}

// helper function to setup the Kafka queue from the CLI arguments.
func setupKafka(c *cli.Context) (queue.Service, error) {
func setupKafka(q *queueSetup) (queue.Service, error) {

Choose a reason for hiding this comment

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

func setupKafka is unused (from unused)

)

// helper function to setup the queue from the CLI arguments.
func setupClient(c *cli.Context) (*vela.Client, error) {
log.Debug("Creating vela client from CLI configuration")
func setupClient(s *Server) (*vela.Client, error) {

Choose a reason for hiding this comment

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

func setupClient is unused (from unused)

logrus.Tracef("Creating %s queue client from CLI configuration", constants.DriverKafka)
// return kafka.New(c.String("queue-config"), "vela")
return nil, fmt.Errorf("unsupported queue driver: %s", constants.DriverKafka)
}

// helper function to setup the Redis queue from the CLI arguments.
func setupRedis(c *cli.Context) (queue.Service, error) {
func setupRedis(q *queueSetup) (queue.Service, error) {

Choose a reason for hiding this comment

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

func setupRedis is unused (from unused)

package main

import (
"github.com/sirupsen/logrus"

Choose a reason for hiding this comment

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

File is not goimports-ed (from goimports)

Suggested change
"github.com/sirupsen/logrus"

tomb := new(tomb.Tomb)

// spawn a tomb goroutine to manage the worker processes
tomb.Go(func() error {

Choose a reason for hiding this comment

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

unnecessary leading newline (from whitespace)

}
}()

for {

Choose a reason for hiding this comment

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

S1000: should use for range instead of for { select {} } (from gosimple)

Copy link
Contributor

@kneal kneal left a comment

Choose a reason for hiding this comment

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

LGTM 🐬

@kneal kneal merged commit 5bb09a8 into master Mar 23, 2020
@kneal kneal deleted the refactor/worker/startup branch March 23, 2020 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Indicates an improvement to a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants