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

Add a custom_process_id parameter for use with lambda #13

Closed
wants to merge 4 commits into from
Closed

Add a custom_process_id parameter for use with lambda #13

wants to merge 4 commits into from

Conversation

genehand
Copy link
Contributor

@genehand genehand commented Feb 5, 2019

I found that using this with a lambda function ended up with the same hostname & PID (inside the containers) for multiple runs. This adds the option to specify a custom_process_id, for example I'm using the context.aws_request_id.

Had to do a little extra to keep rubocop happy :)

@genehand
Copy link
Contributor Author

genehand commented Feb 5, 2019

Looks like there's a new bundler 2.0 issue with Travis: https://docs.travis-ci.com/user/languages/ruby/#bundler-20

lib/marloss/store.rb Outdated Show resolved Hide resolved
lib/marloss/store.rb Outdated Show resolved Hide resolved
@genehand genehand changed the title Add a unique_process_id parameter for use with lambda Add a custom_process_id parameter for use with lambda Feb 5, 2019
@@ -18,7 +21,7 @@
before do
allow(Aws::DynamoDB::Client).to receive(:new).with(client_options)
.and_return(ddb_client)
allow(store).to receive(:`).with("hostname").and_return(hostname)
allow_any_instance_of(described_class).to receive(:`).with("hostname").and_return(hostname)
Copy link
Owner

Choose a reason for hiding this comment

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

why do we need this? we only instantiate the class once at line 14 let(:store) { described_class.new(table, hash_key, options) } i don't understand why we need to use allow_any_instance_of

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When calling host_process_id from initialize, here's what I get without it:

  1) Marloss::Store.create_lock should create the lock
     Failure/Error:
       client.put_item(
         table_name: table,
         item: {
           hash_key => name,
           "ProcessID" => process_id,
           expires_key => (Time.now + ttl).to_i
         },
         expression_attribute_names: {
           "#E" => expires_key,
           "#P" => "ProcessID"

       #<InstanceDouble(Aws::DynamoDB::Client) (anonymous)> received :put_item with unexpected arguments
         expected: ({:condition_expression=>"attribute_not_exists(LockID) OR #E < :now OR #P = :process_id", :expression_...>1549560099, "LockID"=>"my_resource", "ProcessID"=>"hostname.local:86776"}, :table_name=>"my_table"})
              got: ({:condition_expression=>"attribute_not_exists(LockID) OR #E < :now OR #P = :process_id", :expression_...=>1549560099, "LockID"=>"my_resource", "ProcessID"=>"doxbook.local:86776"}, :table_name=>"my_table"})
       Diff:

       @@ -2,10 +2,10 @@
           "attribute_not_exists(LockID) OR #E < :now OR #P = :process_id",
          :expression_attribute_names=>{"#E"=>"Expires", "#P"=>"ProcessID"},
          :expression_attribute_values=>
       -   {":now"=>1549560089, ":process_id"=>"hostname.local:86776"},
       +   {":now"=>1549560089, ":process_id"=>"doxbook.local:86776"},
          :item=>
           {"Expires"=>1549560099,
            "LockID"=>"my_resource",
       -    "ProcessID"=>"hostname.local:86776"},
       +    "ProcessID"=>"doxbook.local:86776"},
          :table_name=>"my_table"}]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had a more experienced rubyist take a look.. when you do allow(store) that calls the let(:store), that instantiates the object but at that point the stub method hasn't been created because it's evaluating the store value to pass to allow()

Copy link
Owner

Choose a reason for hiding this comment

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

gotcha. i pulled down the fork and ran it myself and yes is true, because we now call host_process_id at initialisation the system call get executed before the stubbing. makes sense now 👍
i just think allow_instance_of tend to make specs flaky and cause side effect - i like to avoid using it when possible but in this case is 🆗
thanks for trying this out 🙇

@eredi93
Copy link
Owner

eredi93 commented Feb 8, 2019

@genehand i merged #14 and updated the the ruby versions is Travis. my build was 🍏 can you try rebasing? the bundler error should now be gone 😄

the PR overall looks good happy to merge this in once is 🍏

@genehand
Copy link
Contributor Author

genehand commented Feb 8, 2019

Awesome, travis isn't starting up so I'm going to try closing & reopening

@genehand genehand closed this Feb 8, 2019
@genehand genehand reopened this Feb 8, 2019
@genehand
Copy link
Contributor Author

genehand commented Feb 8, 2019

Hmm, can you kick travis off? Or should I open a new PR?

@eredi93
Copy link
Owner

eredi93 commented Feb 9, 2019

@genehand you need to rebase your repo
as you can see https://github.com/genehand/marloss/blob/master/.travis.yml you still the old trevis file
unfortunately i cannot rebase this PR as i do not have write access to your fork

you would need to rebase like this:

$ git remote add eredi93 git@github.com:eredi93/marloss.git
$ git remote show
eredi93
origin
$ git checkout unique-process-id
$ git pull eredi93 master
$ git rebase eredi93/master
$ git push -f

this should do the trick 😄

@genehand
Copy link
Contributor Author

Hmm, already rebased. You can see the force-push up above and the updated .travis.yml in the branch I opened this from: https://github.com/genehand/marloss/blob/unique-process-id/.travis.yml

Not sure what's up with travis 🤷‍♂️ I'll try with a new PR

@genehand genehand closed this Feb 11, 2019
@genehand genehand deleted the unique-process-id branch February 11, 2019 17:54
@genehand genehand restored the unique-process-id branch February 11, 2019 18:37
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.

None yet

2 participants