Skip to content
This repository has been archived by the owner on Dec 9, 2022. It is now read-only.

LOG-1772: Canoe: productionise Paddle #2

Merged
merged 12 commits into from
Oct 16, 2017

Conversation

pedrocunha
Copy link
Contributor

Still work in progress, but just opening the PR to create some basic comments

===

Jira story #LOG-1772 in project Logistics:

We have created https://github.com/deliveroo/paddle as a proof of concept.

Start master from scratch, and issue a PR with the current functionality and proper tests.

The expected functionality is:

README.md Outdated

```
$ go build
```
Copy link
Contributor

Choose a reason for hiding this comment

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

we should specify that Go must be installed only for development / tests. for actual usage, we'll be generating a binary which we can include instructions in the Readme for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

cli/data/get.go Outdated
path string
}

func (s *S3Target) copy() *S3Target {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this here to ensure that we aren't mutating?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand enough yet about Go to grasp if I should be just passing structs by value and allow the runtime to clone those from my understanding but yet.. here was just do that scenario where you want to be a good citzen:

class A
  def initialize(options = {})
     opts = options.dup
     opts.merge(...)
  end
end

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I wouldn't have this function, if anything we can just copy below; and copy is a built-in so it's not a great name. I would just pass by value, no reason to pass by reference here.

Copy link
Collaborator

@me me left a comment

Choose a reason for hiding this comment

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

LGTM!

cli/data/get.go Outdated
source.path = readHEAD(session, source)
}

fmt.Println("Copying " + source.fullPath() + " to " + destination)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I kinda liked a more minimal output to be honest, no biggie though

aws_secret_access_key=yyy
region=eu-west-1
```

Choose a reason for hiding this comment

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

unnecessary newline

Copy link
Collaborator

@me me left a comment

Choose a reason for hiding this comment

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

Looks good!

We should probably add instructions to the README to create a new release (and create one once this is merge). The steps are:

  • Set up github export GITHUB_TOKEN=[YOUR_TOKEN]
  • $ git tag -a v0.2.0 -m "[Comment]"
  • $ git push origin v0.2.0
  • $ goreleaser

(see https://goreleaser.com)


```
> cat $HOME/.paddle.yaml
bucket: roo-bucket
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be worth pointing out that these settings can be configured through ENV as well (e.g. BUCKET).

README.md Outdated
@@ -21,6 +21,8 @@ You will need create a `$HOME/.paddle.yaml` that contains the bucket name, e.g:
bucket: roo-bucket
```

or if you prefer specific `BUCKET` as an environment variable
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo: "specify"

@pedrocunha pedrocunha merged commit d1aba39 into master Oct 16, 2017
@pedrocunha pedrocunha deleted the LOG-1772/canoe-productionise-paddle branch October 16, 2017 14:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants